Skip to content

Commit

Permalink
session: Stop using php_combined_lcg()
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
TimWolla authored and Girgias committed Mar 2, 2024
1 parent 4df911e commit f6c38fc
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 6 deletions.
3 changes: 3 additions & 0 deletions ext/session/php_session.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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;
Expand Down
21 changes: 15 additions & 6 deletions ext/session/session.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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);
}
/* }}} */

Expand Down

0 comments on commit f6c38fc

Please sign in to comment.