Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:56700 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 76480 invoked from network); 1 Dec 2011 07:09:11 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 1 Dec 2011 07:09:11 -0000 Authentication-Results: pb1.pair.com header.from=dk@uw.no; sender-id=unknown Authentication-Results: pb1.pair.com smtp.mail=dk@uw.no; spf=permerror; sender-id=unknown Received-SPF: error (pb1.pair.com: domain uw.no from 195.159.98.120 cause and error) X-PHP-List-Original-Sender: dk@uw.no X-Host-Fingerprint: 195.159.98.120 in.cluded.net Linux 2.4/2.6 Received: from [195.159.98.120] ([195.159.98.120:50675] helo=in.cluded.net) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 37/10-10027-21827DE4 for ; Thu, 01 Dec 2011 02:09:09 -0500 Received: (qmail 4261 invoked from network); 30 Nov 2011 10:23:55 -0000 Received: from 78.225.251.212.customer.cdi.no (HELO ?10.0.0.100?) (daniel@212.251.225.78) by in.cluded.net with ESMTPA; 30 Nov 2011 10:23:55 -0000 Message-ID: <4ED727FB.7030001@uw.no> Date: Thu, 01 Dec 2011 07:08:43 +0000 User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060307 SeaMonkey/1.5a MIME-Version: 1.0 To: Yasuo Ohgaki CC: Rui Hirokawa , internals@lists.php.net References: <4EBDC283.3040700@yahoo.co.jp> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [PHP-DEV] Strict session? From: dk@uw.no ("Daniel K.") 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.