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.
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
