From f6c38fc952dd0112a7f96f30e652b9418cf1de8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 1 Mar 2024 18:15:48 +0100 Subject: [PATCH] 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. --- ext/session/php_session.h | 3 +++ ext/session/session.c | 21 +++++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/ext/session/php_session.h b/ext/session/php_session.h index 1cb2b2276f6..c9611e61597 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -19,6 +19,7 @@ #include "ext/standard/php_var.h" #include "ext/hash/php_hash.h" +#include "ext/random/php_random.h" #define PHP_SESSION_API 20161017 @@ -157,6 +158,8 @@ typedef struct _php_ps_globals { zend_string *session_started_filename; uint32_t session_started_lineno; int module_number; + php_random_status_state_pcgoneseq128xslrr64 random_state; + php_random_algo_with_state random; zend_long gc_probability; zend_long gc_divisor; zend_long gc_maxlifetime; diff --git a/ext/session/session.c b/ext/session/session.c index 61d91b77eaa..ba4bb661976 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -387,17 +387,16 @@ PHPAPI zend_result php_session_valid_key(const char *key) /* {{{ */ static zend_long php_session_gc(bool immediate) /* {{{ */ { - int nrand; zend_long num = -1; + bool collect = immediate; /* GC must be done before reading session data. */ if ((PS(mod_data) || PS(mod_user_implemented))) { - if (immediate) { - PS(mod)->s_gc(&PS(mod_data), PS(gc_maxlifetime), &num); - return num; + if (!collect && PS(gc_probability) > 0) { + collect = php_random_range(PS(random), 0, PS(gc_divisor) - 1) < PS(gc_probability); } - nrand = (zend_long) ((float) PS(gc_divisor) * php_combined_lcg()); - if (PS(gc_probability) > 0 && nrand < PS(gc_probability)) { + + if (collect) { PS(mod)->s_gc(&PS(mod_data), PS(gc_maxlifetime), &num); } } @@ -2872,6 +2871,16 @@ static PHP_GINIT_FUNCTION(ps) /* {{{ */ ZVAL_UNDEF(&ps_globals->mod_user_names.ps_validate_sid); ZVAL_UNDEF(&ps_globals->mod_user_names.ps_update_timestamp); ZVAL_UNDEF(&ps_globals->http_session_vars); + + ps_globals->random = (php_random_algo_with_state){ + .algo = &php_random_algo_pcgoneseq128xslrr64, + .state = &ps_globals->random_state, + }; + php_random_uint128_t seed; + if (php_random_bytes_silent(&seed, sizeof(seed)) == FAILURE) { + seed = php_random_uint128_constant(GENERATE_SEED(), GENERATE_SEED()); + } + php_random_pcgoneseq128xslrr64_seed128(ps_globals->random.state, seed); } /* }}} */