Re: Strict session?

Yasuo Ohgaki wrote:

I've made the patch for trunk. I checked by using session module tests using valgrind. https://gist.github.com/1379668

As a general comment there seems to be lots of extra white space in the 'empty' lines of the patch.

Index: ext/session/php_session.h =================================================================== --- ext/session/php_session.h (revision 319702) +++ ext/session/php_session.h (working copy) @@ -69,7 +72,7 @@ PS_WRITE_FUNC(x); \ PS_DESTROY_FUNC(x); \ PS_GC_FUNC(x); \ - PS_CREATE_SID_FUNC(x) + PS_CREATE_SID_FUNC(x);

This is bogus, either you meant to do as for PS_FUNCS_SID below; - PS_CREATE_SID_FUNC(x) + PS_CREATE_SID_FUNC(x); \ + PS_VALIDATE_SID_FUNC(x) or you should leave it alone. Your comments below suggests the latter.

@@ -83,11 +86,12 @@ PS_WRITE_FUNC(x); \ PS_DESTROY_FUNC(x); \ PS_GC_FUNC(x); \ - PS_CREATE_SID_FUNC(x) + PS_CREATE_SID_FUNC(x); \ + PS_VALIDATE_SID_FUNC(x) [...] Index: ext/session/mod_user_class.c =================================================================== --- ext/session/mod_user_class.c (revision 319702) +++ ext/session/mod_user_class.c (working copy) @@ -141,4 +141,74 @@ RETVAL_LONG(PS(default_mod)->s_gc(&PS(mod_data), maxlifetime, &nrdels TSRMLS_CC)); } + +static int ps_user_valid_key(const char *key TSRMLS_DC) +{ + size_t len; + const char *p; + char c; + int ret = SUCCESS; + + for (p = key; (c = *p); p++) { + /* valid characters are a..z,A..Z,0..9 */

and ',' and '-', bug copied from mod_files.c?

+ if (!((c >= 'a' && c <= 'z') + || (c >= 'A' && c <= 'Z') + || (c >= '0' && c <= '9') + || c == ',' + || c == '-')) { + ret = FAILURE; + break; + } + } Index: ext/session/mod_mm.c =================================================================== --- ext/session/mod_mm.c (revision 319702) +++ ext/session/mod_mm.c (working copy) @@ -257,6 +257,38 @@ free(data); } +/* If you change the logic here, please also update the error message in + * ps_files_open() appropriately */

Really?

+static int ps_mm_valid_key(const char *key) +{ + size_t len; + const char *p; + char c; + int ret = 1; + + for (p = key; (c = *p); p++) { + /* valid characters are a..z,A..Z,0..9 */

Deja vu.

+ if (!((c >= 'a' && c <= 'z') + || (c >= 'A' && c <= 'Z') + || (c >= '0' && c <= '9') + || c == ',' + || c == '-')) { + ret = 0; + break; + } + } Index: ext/session/mod_user.c =================================================================== --- ext/session/mod_user.c (revision 319702) +++ ext/session/mod_user.c (working copy) @@ -163,6 +163,81 @@ +static int ps_user_valid_key(const char *key TSRMLS_DC) +{ + size_t len; + const char *p; + char c; + int ret = SUCCESS; + + for (p = key; (c = *p); p++) { + /* valid characters are a..z,A..Z,0..9 */

Again.

+ if (!((c >= 'a' && c <= 'z') + || (c >= 'A' && c <= 'Z') + || (c >= '0' && c <= '9') + || c == ',' + || c == '-')) { + ret = FAILURE; + break; + } + } Index: ext/session/session.c =================================================================== --- ext/session/session.c (revision 319702) +++ ext/session/session.c (working copy) @@ -1663,7 +1670,10 @@ /* remove shutdown function */ remove_user_shutdown_function("session_shutdown", sizeof("session_shutdown") TSRMLS_CC); - for (i = 0; i < 6; i++) { + for (i = 0; i < 8; i++) { + if (i >= num_args) { + break; + } @@ -1677,7 +1687,10 @@ zend_alter_ini_entry("session.save_handler", sizeof("session.save_handler"), "user", sizeof("user")-1, PHP_INI_USER, PHP_INI_STAGE_RUNTIME); } - for (i = 0; i < 6; i++) { + for (i = 0; i < 8; i++) { + if (i >= num_args) { + break; + }

What about: for (i = 0, i < num_args, i++)

@@ -2218,9 +2231,13 @@ for (i = 0, mod = ps_modules; i < MAX_MODULES; i++, mod++) { if (*mod && (*mod)->s_name) { smart_str_appends(&save_handlers, (*mod)->s_name); - smart_str_appendc(&save_handlers, ' '); + if ((*mod)->s_validate_sid) { + smart_str_appends(&save_handlers, "(strict) "); + } else { + smart_str_appends(&save_handlers, "(adaptive) "); } } + }

Funky indentation and brace placement.

This patch adds - validate_sid() to ps_modules (Save handlers) - use_strict_session to php.ini (On by default, off for old behavior) - display that save handler supports strict session or not via phpinfo() (So that user could know behavior) - update PHP_SESSION_API version (So that save handler authors could write portable code)

Reading the patch seems to confirm this.

Compatibility issues are - save handlers that are currently working should also work with this patch, except ps modules using PS_MOD_SID and PS_MOD_FUNCS_SID macro.

None of the bundled ones do currently.

These ps modules should implement validate_sid(). Modules that are using PS_MOD/PS_FUNCS are not affected, they just marked as "adaptive" module. (e.g. pecl sqlite's ps module. You can see it via phpinfo())

    NOTE: PS_MOD_SID() and PS_MOD_FUNCS_SID() are already defined, but

it was the same as PS_MOD()/PS_MOD_FUNCS().

Well, not exactly the same. #define PS_MOD(x) \

       #x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x, \
        ps_delete_##x, ps_gc_##x, php_session_create_id

#define PS_MOD_SID(x) \

       #x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x, \
        ps_delete_##x, ps_gc_##x, ps_create_sid_##x

If old ps_module does not compile, just remove "_SID" from PS_MOD_SID()/PS_MOD_FUNCS_SID().

This is bad advice due to the difference between the SID/non-SID versions.

- session ID string is checked so that chars are alphanumeric + ',' '-' when use_strict_session=On.

php_session_initialize() checks for some additional invalid characters as well. Why be strict and allow only [0-9a-zA-Z,-], different storage backends have different requirements.

(mod_file.c checks session ID this way w/o this patch to prevent problems. Using restrictive session ID string is better for other ps modules. IMHO)

Why is it better? And if it is, could the function be made generic instead of it being copied around, bugs and all?

- session read failure is not rescued to eliminate possible infinite loops. Bundled ps modules were not using this at least.

More on loops below.

You will see some tests are failing since they depend on adaptive session. By looking into failing test results, you can see this patch is generating new session ID if it's not a already initialized session. I'll modify these tests (set use_strict_session=Off) and add some tests for new feature (new validate_sid() function for user and use_class save handler) after commit.

Why not at the same time? It seems appropriate to prove that you have tested the new features when you commit them?

It removes session read failure retry code from php_session_initialize() . The retry code may cause infinite loop depending on save handler implementation. Some session save handlers may be using this to prevent adoption(?)

I do not believe this could have been used as you speculate. The retry logic kicks in when PS_READ_FUNC() fails _and_ additionally sets PS(invalid_session_id) This could never work with: session_id("foo"); session_start(); could it? Have you checked that this still works as advertised with the patch applied? I may have additional comments if I can find time to test it. Daniel K.