Skip to content

Commit f6c38fc

Browse files
TimWollaGirgias
authored andcommitted
session: Stop using php_combined_lcg()
The CombinedLCG is a terrible RNG with a questionable API and should ideally not be used anymore. While in the case of ext/session it is only used for probabilistic garbage collection where the quality of the RNG is not of particular importance, there are better choices. Replace the RNG used for garbage collection by an ext/session specific instance of PcgOneseq128XslRr64. Its 16 Byte state nicely fits into the memory freed up by the previous reordering of the session globals struct, even allowing to the storage of the php_random_algo_with_state struct, making using the RNG a little nicer. Instead multiplying the float returned by the CombinedLCG by the GC Divisor to obtain an integer between 0 and the divisor we can just use `php_random_range` to directly generate an appropriate integer, completely avoiding the floating point maths, making it easier to verify the code for correctness.
1 parent 4df911e commit f6c38fc

File tree

2 files changed

+18
-6
lines changed

2 files changed

+18
-6
lines changed

ext/session/php_session.h

+3
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include "ext/standard/php_var.h"
2121
#include "ext/hash/php_hash.h"
22+
#include "ext/random/php_random.h"
2223

2324
#define PHP_SESSION_API 20161017
2425

@@ -157,6 +158,8 @@ typedef struct _php_ps_globals {
157158
zend_string *session_started_filename;
158159
uint32_t session_started_lineno;
159160
int module_number;
161+
php_random_status_state_pcgoneseq128xslrr64 random_state;
162+
php_random_algo_with_state random;
160163
zend_long gc_probability;
161164
zend_long gc_divisor;
162165
zend_long gc_maxlifetime;

ext/session/session.c

+15-6
Original file line numberDiff line numberDiff line change
@@ -387,17 +387,16 @@ PHPAPI zend_result php_session_valid_key(const char *key) /* {{{ */
387387

388388
static zend_long php_session_gc(bool immediate) /* {{{ */
389389
{
390-
int nrand;
391390
zend_long num = -1;
391+
bool collect = immediate;
392392

393393
/* GC must be done before reading session data. */
394394
if ((PS(mod_data) || PS(mod_user_implemented))) {
395-
if (immediate) {
396-
PS(mod)->s_gc(&PS(mod_data), PS(gc_maxlifetime), &num);
397-
return num;
395+
if (!collect && PS(gc_probability) > 0) {
396+
collect = php_random_range(PS(random), 0, PS(gc_divisor) - 1) < PS(gc_probability);
398397
}
399-
nrand = (zend_long) ((float) PS(gc_divisor) * php_combined_lcg());
400-
if (PS(gc_probability) > 0 && nrand < PS(gc_probability)) {
398+
399+
if (collect) {
401400
PS(mod)->s_gc(&PS(mod_data), PS(gc_maxlifetime), &num);
402401
}
403402
}
@@ -2872,6 +2871,16 @@ static PHP_GINIT_FUNCTION(ps) /* {{{ */
28722871
ZVAL_UNDEF(&ps_globals->mod_user_names.ps_validate_sid);
28732872
ZVAL_UNDEF(&ps_globals->mod_user_names.ps_update_timestamp);
28742873
ZVAL_UNDEF(&ps_globals->http_session_vars);
2874+
2875+
ps_globals->random = (php_random_algo_with_state){
2876+
.algo = &php_random_algo_pcgoneseq128xslrr64,
2877+
.state = &ps_globals->random_state,
2878+
};
2879+
php_random_uint128_t seed;
2880+
if (php_random_bytes_silent(&seed, sizeof(seed)) == FAILURE) {
2881+
seed = php_random_uint128_constant(GENERATE_SEED(), GENERATE_SEED());
2882+
}
2883+
php_random_pcgoneseq128xslrr64_seed128(ps_globals->random.state, seed);
28752884
}
28762885
/* }}} */
28772886

0 commit comments

Comments
 (0)