From 6230c2bad089bfbf518b64ef0868bf9d55a2145c Mon Sep 17 00:00:00 2001 From: Yasuo Ohgaki Date: Thu, 10 Nov 2016 16:03:41 +0900 Subject: [PATCH 1/8] Fix Bug #73461 This patch disables any invalid save handler calls. --- ext/session/mod_user.c | 82 +++++++++++++++++++++++++ ext/session/php_session.h | 1 + ext/session/session.c | 3 +- ext/session/tests/bug60634.phpt | 16 ++--- ext/session/tests/bug60634_error_3.phpt | 3 +- ext/session/tests/bug60634_error_4.phpt | 2 +- 6 files changed, 96 insertions(+), 11 deletions(-) diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c index 0cdbaf96f9d..0a54204cc9d 100644 --- a/ext/session/mod_user.c +++ b/ext/session/mod_user.c @@ -75,7 +75,15 @@ PS_OPEN_FUNC(user) zval args[2]; STDVARS; + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return FAILURE; + } + PS(in_save_handler) = 1; + if (Z_ISUNDEF(PSF(open))) { + PS(in_save_handler) = 0; php_error_docref(NULL, E_WARNING, "user session functions not defined"); @@ -88,6 +96,7 @@ PS_OPEN_FUNC(user) zend_try { ps_call_handler(&PSF(open), 2, args, &retval); } zend_catch { + PS(in_save_handler) = 0; PS(session_status) = php_session_none; if (!Z_ISUNDEF(retval)) { zval_ptr_dtor(&retval); @@ -97,6 +106,7 @@ PS_OPEN_FUNC(user) PS(mod_user_implemented) = 1; + PS(in_save_handler) = 0; FINISH; } @@ -105,8 +115,16 @@ PS_CLOSE_FUNC(user) zend_bool bailout = 0; STDVARS; + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return FAILURE; + } + PS(in_save_handler) = 1; + if (!PS(mod_user_implemented)) { /* already closed */ + PS(in_save_handler) = 0; return SUCCESS; } @@ -119,12 +137,14 @@ PS_CLOSE_FUNC(user) PS(mod_user_implemented) = 0; if (bailout) { + PS(in_save_handler) = 0; if (!Z_ISUNDEF(retval)) { zval_ptr_dtor(&retval); } zend_bailout(); } + PS(in_save_handler) = 0; FINISH; } @@ -133,6 +153,13 @@ PS_READ_FUNC(user) zval args[1]; STDVARS; + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return FAILURE; + } + PS(in_save_handler) = 1; + ZVAL_STR_COPY(&args[0], key); ps_call_handler(&PSF(read), 1, args, &retval); @@ -145,6 +172,7 @@ PS_READ_FUNC(user) zval_ptr_dtor(&retval); } + PS(in_save_handler) = 0; return ret; } @@ -153,11 +181,19 @@ PS_WRITE_FUNC(user) zval args[2]; STDVARS; + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return FAILURE; + } + PS(in_save_handler) = 1; + ZVAL_STR_COPY(&args[0], key); ZVAL_STR_COPY(&args[1], val); ps_call_handler(&PSF(write), 2, args, &retval); + PS(in_save_handler) = 0; FINISH; } @@ -166,10 +202,18 @@ PS_DESTROY_FUNC(user) zval args[1]; STDVARS; + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return FAILURE; + } + PS(in_save_handler) = 1; + ZVAL_STR_COPY(&args[0], key); ps_call_handler(&PSF(destroy), 1, args, &retval); + PS(in_save_handler) = 0; FINISH; } @@ -178,24 +222,41 @@ PS_GC_FUNC(user) zval args[1]; zval retval; + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return FAILURE; + } + PS(in_save_handler) = 1; + ZVAL_LONG(&args[0], maxlifetime); ps_call_handler(&PSF(gc), 1, args, &retval); if (Z_TYPE(retval) == IS_LONG) { convert_to_long(&retval); + PS(in_save_handler) = 0; return Z_LVAL(retval); } /* This is for older API compatibility */ if (Z_TYPE(retval) == IS_TRUE) { + PS(in_save_handler) = 0; return 1; } + PS(in_save_handler) = 0; /* Anything else is some kind of error */ return -1; // Error } PS_CREATE_SID_FUNC(user) { + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return FAILURE; + } + PS(in_save_handler) = 1; + /* maintain backwards compatibility */ if (!Z_ISUNDEF(PSF(create_sid))) { zend_string *id = NULL; @@ -209,24 +270,35 @@ PS_CREATE_SID_FUNC(user) } zval_ptr_dtor(&retval); } else { + PS(in_save_handler) = 0; zend_throw_error(NULL, "No session id returned by function"); return NULL; } if (!id) { + PS(in_save_handler) = 0; zend_throw_error(NULL, "Session id must be a string"); return NULL; } + PS(in_save_handler) = 0; return id; } + PS(in_save_handler) = 0; /* function as defined by PS_MOD */ return php_session_create_id(mod_data); } PS_VALIDATE_SID_FUNC(user) { + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return FAILURE; + } + PS(in_save_handler) = 1; + /* maintain backwards compatibility */ if (!Z_ISUNDEF(PSF(validate_sid))) { zval args[1]; @@ -236,9 +308,11 @@ PS_VALIDATE_SID_FUNC(user) ps_call_handler(&PSF(validate_sid), 1, args, &retval); + PS(in_save_handler) = 0; FINISH; } + PS(in_save_handler) = 0; /* dummy function defined by PS_MOD */ return php_session_validate_sid(mod_data, key); } @@ -248,6 +322,13 @@ PS_UPDATE_TIMESTAMP_FUNC(user) zval args[2]; STDVARS; + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return FAILURE; + } + PS(in_save_handler) = 1; + ZVAL_STR_COPY(&args[0], key); ZVAL_STR_COPY(&args[1], val); @@ -258,6 +339,7 @@ PS_UPDATE_TIMESTAMP_FUNC(user) ps_call_handler(&PSF(write), 2, args, &retval); } + PS(in_save_handler) = 0; FINISH; } diff --git a/ext/session/php_session.h b/ext/session/php_session.h index da5e48515a4..775527c2f61 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -205,6 +205,7 @@ typedef struct _php_ps_globals { zend_bool use_strict_mode; /* whether or not PHP accepts unknown session ids */ zend_bool lazy_write; /* omit session write when it is possible */ zend_string *session_vars; /* serialized original session data */ + zend_bool in_save_handler; /* state that if session is in save handler or not */ } php_ps_globals; typedef php_ps_globals zend_ps_globals; diff --git a/ext/session/session.c b/ext/session/session.c index 5484390c7e9..bbc531aa730 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -106,6 +106,7 @@ static inline void php_rinit_session_globals(void) /* {{{ */ /* TODO: These could be moved to MINIT and removed. These should be initialized by php_rshutdown_session_globals() always when execution is finished. */ PS(id) = NULL; PS(session_status) = php_session_none; + PS(in_save_handler) = 0; PS(mod_data) = NULL; PS(mod_user_is_open) = 0; PS(define_sid) = 1; @@ -2035,7 +2036,7 @@ static PHP_FUNCTION(session_create_id) } } - if (PS(session_status) == php_session_active) { + if (!PS(in_save_handler) && PS(session_status) == php_session_active) { int limit = 3; while (limit--) { new_id = PS(mod)->s_create_sid(&PS(mod_data)); diff --git a/ext/session/tests/bug60634.phpt b/ext/session/tests/bug60634.phpt index b2f50762872..c21c6360a6a 100644 --- a/ext/session/tests/bug60634.phpt +++ b/ext/session/tests/bug60634.phpt @@ -40,16 +40,16 @@ session_write_close(); echo "um, hi\n"; /* -FIXME: Since session module try to write/close session data in -RSHUTDOWN, write() is executed twices. This is caused by undefined -function error and zend_bailout(). Current session module codes -depends on this behavior. These codes should be modified to remove -multiple write(). -*/ + * This test raises error in Unknown function because 2nd close write handler is + * called at request shutdown and session module detects recursive call like + * multiple save handler calls. + */ ?> --EXPECTF-- write: goodbye cruel world -write: goodbye cruel world -close: goodbye cruel world +Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 + +Warning: Unknown: Failed to write session data using user defined save handler. (session.save_path: ) in Unknown on line 0 +close: goodbye cruel world diff --git a/ext/session/tests/bug60634_error_3.phpt b/ext/session/tests/bug60634_error_3.phpt index b7840b04f9d..b38899b8c12 100644 --- a/ext/session/tests/bug60634_error_3.phpt +++ b/ext/session/tests/bug60634_error_3.phpt @@ -48,4 +48,5 @@ Stack trace: #0 [internal function]: write(%s, '') #1 {main} thrown in %s on line %d -close: goodbye cruel world + +Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 diff --git a/ext/session/tests/bug60634_error_4.phpt b/ext/session/tests/bug60634_error_4.phpt index 7970b35c7a8..3ac62479747 100644 --- a/ext/session/tests/bug60634_error_4.phpt +++ b/ext/session/tests/bug60634_error_4.phpt @@ -48,5 +48,5 @@ Stack trace: #0 [internal function]: write('%s', '') #1 {main} thrown in %s on line %d -close: goodbye cruel world +Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 From 46c64ac9f2b08b0367b4e05683ed5029d1636ed9 Mon Sep 17 00:00:00 2001 From: Yasuo Ohgaki Date: Fri, 11 Nov 2016 12:18:54 +0900 Subject: [PATCH 2/8] Protect class based session save handler --- ext/session/mod_user_class.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/ext/session/mod_user_class.c b/ext/session/mod_user_class.c index b132552fafa..595338d299f 100644 --- a/ext/session/mod_user_class.c +++ b/ext/session/mod_user_class.c @@ -53,6 +53,7 @@ PHP_METHOD(SessionHandler, open) } PS(mod_user_is_open) = 1; + PS(in_save_handler) = 1; zend_try { ret = PS(default_mod)->s_open(&PS(mod_data), save_path, session_name); @@ -61,6 +62,7 @@ PHP_METHOD(SessionHandler, open) zend_bailout(); } zend_end_try(); + PS(in_save_handler) = 0; RETVAL_BOOL(SUCCESS == ret); } /* }}} */ @@ -78,6 +80,7 @@ PHP_METHOD(SessionHandler, close) zend_parse_parameters_none(); PS(mod_user_is_open) = 0; + PS(in_save_handler) = 1; zend_try { ret = PS(default_mod)->s_close(&PS(mod_data)); @@ -86,6 +89,7 @@ PHP_METHOD(SessionHandler, close) zend_bailout(); } zend_end_try(); + PS(in_save_handler) = 0; RETVAL_BOOL(SUCCESS == ret); } /* }}} */ @@ -116,6 +120,7 @@ PHP_METHOD(SessionHandler, read) PHP_METHOD(SessionHandler, write) { zend_string *key, *val; + zend_bool ret; PS_SANITY_CHECK_IS_OPEN; @@ -123,7 +128,11 @@ PHP_METHOD(SessionHandler, write) return; } - RETURN_BOOL(SUCCESS == PS(default_mod)->s_write(&PS(mod_data), key, val, PS(gc_maxlifetime))); + PS(in_save_handler) = 1; + ret = PS(default_mod)->s_write(&PS(mod_data), key, val, PS(gc_maxlifetime)); + PS(in_save_handler) = 0; + + RETURN_BOOL(SUCCESS == ret); } /* }}} */ @@ -132,6 +141,7 @@ PHP_METHOD(SessionHandler, write) PHP_METHOD(SessionHandler, destroy) { zend_string *key; + zend_bool ret; PS_SANITY_CHECK_IS_OPEN; @@ -139,7 +149,11 @@ PHP_METHOD(SessionHandler, destroy) return; } - RETURN_BOOL(SUCCESS == PS(default_mod)->s_destroy(&PS(mod_data), key)); + PS(in_save_handler) = 1; + ret = PS(default_mod)->s_destroy(&PS(mod_data), key); + PS(in_save_handler) = 0; + + RETURN_BOOL(SUCCESS == ret); } /* }}} */ @@ -156,9 +170,12 @@ PHP_METHOD(SessionHandler, gc) return; } + PS(in_save_handler) = 1; if (PS(default_mod)->s_gc(&PS(mod_data), maxlifetime, &nrdels) == FAILURE) { + PS(in_save_handler) = 0; RETURN_FALSE; } + PS(in_save_handler) = 0; RETURN_LONG(nrdels); } /* }}} */ @@ -175,7 +192,9 @@ PHP_METHOD(SessionHandler, create_sid) return; } + PS(in_save_handler) = 1; id = PS(default_mod)->s_create_sid(&PS(mod_data)); + PS(in_save_handler) = 0; RETURN_STR(id); } @@ -203,6 +222,7 @@ PHP_METHOD(SessionHandler, validateId) PHP_METHOD(SessionHandler, updateTimestamp) { zend_string *key, *val; + zend_bool ret; PS_SANITY_CHECK_IS_OPEN; @@ -210,7 +230,11 @@ PHP_METHOD(SessionHandler, updateTimestamp) return; } + PS(in_save_handler) = 1; + ret = PS(default_mod)->s_write(&PS(mod_data), key, val, PS(gc_maxlifetime)); + PS(in_save_handler) = 0; + /* Legacy save handler may not support update_timestamp API. Just write. */ - RETVAL_BOOL(SUCCESS == PS(default_mod)->s_write(&PS(mod_data), key, val, PS(gc_maxlifetime))); + RETVAL_BOOL(SUCCESS == ret); } /* }}} */ From 186ff85588abc609bdd3438386887f70ad5c4f9d Mon Sep 17 00:00:00 2001 From: Yasuo Ohgaki Date: Fri, 11 Nov 2016 12:52:17 +0900 Subject: [PATCH 3/8] Revert "Protect class based session save handler" This reverts commit d1be861aee1287e8ecb04b72d93b9749ab67706e. --- ext/session/mod_user_class.c | 30 +++--------------------------- 1 file changed, 3 insertions(+), 27 deletions(-) diff --git a/ext/session/mod_user_class.c b/ext/session/mod_user_class.c index 595338d299f..b132552fafa 100644 --- a/ext/session/mod_user_class.c +++ b/ext/session/mod_user_class.c @@ -53,7 +53,6 @@ PHP_METHOD(SessionHandler, open) } PS(mod_user_is_open) = 1; - PS(in_save_handler) = 1; zend_try { ret = PS(default_mod)->s_open(&PS(mod_data), save_path, session_name); @@ -62,7 +61,6 @@ PHP_METHOD(SessionHandler, open) zend_bailout(); } zend_end_try(); - PS(in_save_handler) = 0; RETVAL_BOOL(SUCCESS == ret); } /* }}} */ @@ -80,7 +78,6 @@ PHP_METHOD(SessionHandler, close) zend_parse_parameters_none(); PS(mod_user_is_open) = 0; - PS(in_save_handler) = 1; zend_try { ret = PS(default_mod)->s_close(&PS(mod_data)); @@ -89,7 +86,6 @@ PHP_METHOD(SessionHandler, close) zend_bailout(); } zend_end_try(); - PS(in_save_handler) = 0; RETVAL_BOOL(SUCCESS == ret); } /* }}} */ @@ -120,7 +116,6 @@ PHP_METHOD(SessionHandler, read) PHP_METHOD(SessionHandler, write) { zend_string *key, *val; - zend_bool ret; PS_SANITY_CHECK_IS_OPEN; @@ -128,11 +123,7 @@ PHP_METHOD(SessionHandler, write) return; } - PS(in_save_handler) = 1; - ret = PS(default_mod)->s_write(&PS(mod_data), key, val, PS(gc_maxlifetime)); - PS(in_save_handler) = 0; - - RETURN_BOOL(SUCCESS == ret); + RETURN_BOOL(SUCCESS == PS(default_mod)->s_write(&PS(mod_data), key, val, PS(gc_maxlifetime))); } /* }}} */ @@ -141,7 +132,6 @@ PHP_METHOD(SessionHandler, write) PHP_METHOD(SessionHandler, destroy) { zend_string *key; - zend_bool ret; PS_SANITY_CHECK_IS_OPEN; @@ -149,11 +139,7 @@ PHP_METHOD(SessionHandler, destroy) return; } - PS(in_save_handler) = 1; - ret = PS(default_mod)->s_destroy(&PS(mod_data), key); - PS(in_save_handler) = 0; - - RETURN_BOOL(SUCCESS == ret); + RETURN_BOOL(SUCCESS == PS(default_mod)->s_destroy(&PS(mod_data), key)); } /* }}} */ @@ -170,12 +156,9 @@ PHP_METHOD(SessionHandler, gc) return; } - PS(in_save_handler) = 1; if (PS(default_mod)->s_gc(&PS(mod_data), maxlifetime, &nrdels) == FAILURE) { - PS(in_save_handler) = 0; RETURN_FALSE; } - PS(in_save_handler) = 0; RETURN_LONG(nrdels); } /* }}} */ @@ -192,9 +175,7 @@ PHP_METHOD(SessionHandler, create_sid) return; } - PS(in_save_handler) = 1; id = PS(default_mod)->s_create_sid(&PS(mod_data)); - PS(in_save_handler) = 0; RETURN_STR(id); } @@ -222,7 +203,6 @@ PHP_METHOD(SessionHandler, validateId) PHP_METHOD(SessionHandler, updateTimestamp) { zend_string *key, *val; - zend_bool ret; PS_SANITY_CHECK_IS_OPEN; @@ -230,11 +210,7 @@ PHP_METHOD(SessionHandler, updateTimestamp) return; } - PS(in_save_handler) = 1; - ret = PS(default_mod)->s_write(&PS(mod_data), key, val, PS(gc_maxlifetime)); - PS(in_save_handler) = 0; - /* Legacy save handler may not support update_timestamp API. Just write. */ - RETVAL_BOOL(SUCCESS == ret); + RETVAL_BOOL(SUCCESS == PS(default_mod)->s_write(&PS(mod_data), key, val, PS(gc_maxlifetime))); } /* }}} */ From 7b29c3fba6678ea84285aa60b2494cc79f388bbb Mon Sep 17 00:00:00 2001 From: Yasuo Ohgaki Date: Fri, 11 Nov 2016 12:52:31 +0900 Subject: [PATCH 4/8] Revert "Fix Bug #73461" This reverts commit 0383de14678e4c77e11ebf261530d4c1260825a1. --- ext/session/mod_user.c | 82 ------------------------- ext/session/php_session.h | 1 - ext/session/session.c | 3 +- ext/session/tests/bug60634.phpt | 16 ++--- ext/session/tests/bug60634_error_3.phpt | 3 +- ext/session/tests/bug60634_error_4.phpt | 2 +- 6 files changed, 11 insertions(+), 96 deletions(-) diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c index 0a54204cc9d..0cdbaf96f9d 100644 --- a/ext/session/mod_user.c +++ b/ext/session/mod_user.c @@ -75,15 +75,7 @@ PS_OPEN_FUNC(user) zval args[2]; STDVARS; - if (PS(in_save_handler)) { - PS(in_save_handler) = 0; - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); - return FAILURE; - } - PS(in_save_handler) = 1; - if (Z_ISUNDEF(PSF(open))) { - PS(in_save_handler) = 0; php_error_docref(NULL, E_WARNING, "user session functions not defined"); @@ -96,7 +88,6 @@ PS_OPEN_FUNC(user) zend_try { ps_call_handler(&PSF(open), 2, args, &retval); } zend_catch { - PS(in_save_handler) = 0; PS(session_status) = php_session_none; if (!Z_ISUNDEF(retval)) { zval_ptr_dtor(&retval); @@ -106,7 +97,6 @@ PS_OPEN_FUNC(user) PS(mod_user_implemented) = 1; - PS(in_save_handler) = 0; FINISH; } @@ -115,16 +105,8 @@ PS_CLOSE_FUNC(user) zend_bool bailout = 0; STDVARS; - if (PS(in_save_handler)) { - PS(in_save_handler) = 0; - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); - return FAILURE; - } - PS(in_save_handler) = 1; - if (!PS(mod_user_implemented)) { /* already closed */ - PS(in_save_handler) = 0; return SUCCESS; } @@ -137,14 +119,12 @@ PS_CLOSE_FUNC(user) PS(mod_user_implemented) = 0; if (bailout) { - PS(in_save_handler) = 0; if (!Z_ISUNDEF(retval)) { zval_ptr_dtor(&retval); } zend_bailout(); } - PS(in_save_handler) = 0; FINISH; } @@ -153,13 +133,6 @@ PS_READ_FUNC(user) zval args[1]; STDVARS; - if (PS(in_save_handler)) { - PS(in_save_handler) = 0; - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); - return FAILURE; - } - PS(in_save_handler) = 1; - ZVAL_STR_COPY(&args[0], key); ps_call_handler(&PSF(read), 1, args, &retval); @@ -172,7 +145,6 @@ PS_READ_FUNC(user) zval_ptr_dtor(&retval); } - PS(in_save_handler) = 0; return ret; } @@ -181,19 +153,11 @@ PS_WRITE_FUNC(user) zval args[2]; STDVARS; - if (PS(in_save_handler)) { - PS(in_save_handler) = 0; - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); - return FAILURE; - } - PS(in_save_handler) = 1; - ZVAL_STR_COPY(&args[0], key); ZVAL_STR_COPY(&args[1], val); ps_call_handler(&PSF(write), 2, args, &retval); - PS(in_save_handler) = 0; FINISH; } @@ -202,18 +166,10 @@ PS_DESTROY_FUNC(user) zval args[1]; STDVARS; - if (PS(in_save_handler)) { - PS(in_save_handler) = 0; - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); - return FAILURE; - } - PS(in_save_handler) = 1; - ZVAL_STR_COPY(&args[0], key); ps_call_handler(&PSF(destroy), 1, args, &retval); - PS(in_save_handler) = 0; FINISH; } @@ -222,41 +178,24 @@ PS_GC_FUNC(user) zval args[1]; zval retval; - if (PS(in_save_handler)) { - PS(in_save_handler) = 0; - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); - return FAILURE; - } - PS(in_save_handler) = 1; - ZVAL_LONG(&args[0], maxlifetime); ps_call_handler(&PSF(gc), 1, args, &retval); if (Z_TYPE(retval) == IS_LONG) { convert_to_long(&retval); - PS(in_save_handler) = 0; return Z_LVAL(retval); } /* This is for older API compatibility */ if (Z_TYPE(retval) == IS_TRUE) { - PS(in_save_handler) = 0; return 1; } - PS(in_save_handler) = 0; /* Anything else is some kind of error */ return -1; // Error } PS_CREATE_SID_FUNC(user) { - if (PS(in_save_handler)) { - PS(in_save_handler) = 0; - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); - return FAILURE; - } - PS(in_save_handler) = 1; - /* maintain backwards compatibility */ if (!Z_ISUNDEF(PSF(create_sid))) { zend_string *id = NULL; @@ -270,35 +209,24 @@ PS_CREATE_SID_FUNC(user) } zval_ptr_dtor(&retval); } else { - PS(in_save_handler) = 0; zend_throw_error(NULL, "No session id returned by function"); return NULL; } if (!id) { - PS(in_save_handler) = 0; zend_throw_error(NULL, "Session id must be a string"); return NULL; } - PS(in_save_handler) = 0; return id; } - PS(in_save_handler) = 0; /* function as defined by PS_MOD */ return php_session_create_id(mod_data); } PS_VALIDATE_SID_FUNC(user) { - if (PS(in_save_handler)) { - PS(in_save_handler) = 0; - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); - return FAILURE; - } - PS(in_save_handler) = 1; - /* maintain backwards compatibility */ if (!Z_ISUNDEF(PSF(validate_sid))) { zval args[1]; @@ -308,11 +236,9 @@ PS_VALIDATE_SID_FUNC(user) ps_call_handler(&PSF(validate_sid), 1, args, &retval); - PS(in_save_handler) = 0; FINISH; } - PS(in_save_handler) = 0; /* dummy function defined by PS_MOD */ return php_session_validate_sid(mod_data, key); } @@ -322,13 +248,6 @@ PS_UPDATE_TIMESTAMP_FUNC(user) zval args[2]; STDVARS; - if (PS(in_save_handler)) { - PS(in_save_handler) = 0; - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); - return FAILURE; - } - PS(in_save_handler) = 1; - ZVAL_STR_COPY(&args[0], key); ZVAL_STR_COPY(&args[1], val); @@ -339,7 +258,6 @@ PS_UPDATE_TIMESTAMP_FUNC(user) ps_call_handler(&PSF(write), 2, args, &retval); } - PS(in_save_handler) = 0; FINISH; } diff --git a/ext/session/php_session.h b/ext/session/php_session.h index 775527c2f61..da5e48515a4 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -205,7 +205,6 @@ typedef struct _php_ps_globals { zend_bool use_strict_mode; /* whether or not PHP accepts unknown session ids */ zend_bool lazy_write; /* omit session write when it is possible */ zend_string *session_vars; /* serialized original session data */ - zend_bool in_save_handler; /* state that if session is in save handler or not */ } php_ps_globals; typedef php_ps_globals zend_ps_globals; diff --git a/ext/session/session.c b/ext/session/session.c index bbc531aa730..5484390c7e9 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -106,7 +106,6 @@ static inline void php_rinit_session_globals(void) /* {{{ */ /* TODO: These could be moved to MINIT and removed. These should be initialized by php_rshutdown_session_globals() always when execution is finished. */ PS(id) = NULL; PS(session_status) = php_session_none; - PS(in_save_handler) = 0; PS(mod_data) = NULL; PS(mod_user_is_open) = 0; PS(define_sid) = 1; @@ -2036,7 +2035,7 @@ static PHP_FUNCTION(session_create_id) } } - if (!PS(in_save_handler) && PS(session_status) == php_session_active) { + if (PS(session_status) == php_session_active) { int limit = 3; while (limit--) { new_id = PS(mod)->s_create_sid(&PS(mod_data)); diff --git a/ext/session/tests/bug60634.phpt b/ext/session/tests/bug60634.phpt index c21c6360a6a..b2f50762872 100644 --- a/ext/session/tests/bug60634.phpt +++ b/ext/session/tests/bug60634.phpt @@ -40,16 +40,16 @@ session_write_close(); echo "um, hi\n"; /* - * This test raises error in Unknown function because 2nd close write handler is - * called at request shutdown and session module detects recursive call like - * multiple save handler calls. - */ +FIXME: Since session module try to write/close session data in +RSHUTDOWN, write() is executed twices. This is caused by undefined +function error and zend_bailout(). Current session module codes +depends on this behavior. These codes should be modified to remove +multiple write(). +*/ ?> --EXPECTF-- write: goodbye cruel world - -Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 - -Warning: Unknown: Failed to write session data using user defined save handler. (session.save_path: ) in Unknown on line 0 +write: goodbye cruel world close: goodbye cruel world + diff --git a/ext/session/tests/bug60634_error_3.phpt b/ext/session/tests/bug60634_error_3.phpt index b38899b8c12..b7840b04f9d 100644 --- a/ext/session/tests/bug60634_error_3.phpt +++ b/ext/session/tests/bug60634_error_3.phpt @@ -48,5 +48,4 @@ Stack trace: #0 [internal function]: write(%s, '') #1 {main} thrown in %s on line %d - -Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 +close: goodbye cruel world diff --git a/ext/session/tests/bug60634_error_4.phpt b/ext/session/tests/bug60634_error_4.phpt index 3ac62479747..7970b35c7a8 100644 --- a/ext/session/tests/bug60634_error_4.phpt +++ b/ext/session/tests/bug60634_error_4.phpt @@ -48,5 +48,5 @@ Stack trace: #0 [internal function]: write('%s', '') #1 {main} thrown in %s on line %d +close: goodbye cruel world -Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 From 3d6e922367fc85fd175394b53946604807466011 Mon Sep 17 00:00:00 2001 From: Yasuo Ohgaki Date: Tue, 15 Nov 2016 11:12:26 +0900 Subject: [PATCH 5/8] Refactor and cleanup implementation. --- ext/session/mod_user.c | 8 ++++++++ ext/session/php_session.h | 1 + ext/session/session.c | 11 +++++++---- ext/session/tests/bug60634.phpt | 19 +++++++++++-------- ext/session/tests/bug60634_error_3.phpt | 3 ++- ext/session/tests/bug60634_error_4.phpt | 2 +- ext/session/tests/bug69111.phpt | 7 +++++-- ext/session/tests/bug71162.phpt | 6 +++--- 8 files changed, 38 insertions(+), 19 deletions(-) diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c index 0cdbaf96f9d..9755f0d5c49 100644 --- a/ext/session/mod_user.c +++ b/ext/session/mod_user.c @@ -30,12 +30,20 @@ ps_module ps_mod_user = { static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval) { int i; + if (PS(in_save_handler)) { + PS(in_save_handler) = 0; + ZVAL_UNDEF(retval); + php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + return; + } + PS(in_save_handler) = 1; if (call_user_function(EG(function_table), NULL, func, retval, argc, argv) == FAILURE) { zval_ptr_dtor(retval); ZVAL_UNDEF(retval); } else if (Z_ISUNDEF_P(retval)) { ZVAL_NULL(retval); } + PS(in_save_handler) = 0; for (i = 0; i < argc; i++) { zval_ptr_dtor(&argv[i]); } diff --git a/ext/session/php_session.h b/ext/session/php_session.h index da5e48515a4..775527c2f61 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -205,6 +205,7 @@ typedef struct _php_ps_globals { zend_bool use_strict_mode; /* whether or not PHP accepts unknown session ids */ zend_bool lazy_write; /* omit session write when it is possible */ zend_string *session_vars; /* serialized original session data */ + zend_bool in_save_handler; /* state that if session is in save handler or not */ } php_ps_globals; typedef php_ps_globals zend_ps_globals; diff --git a/ext/session/session.c b/ext/session/session.c index 5484390c7e9..ecce0c322cf 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -106,6 +106,7 @@ static inline void php_rinit_session_globals(void) /* {{{ */ /* TODO: These could be moved to MINIT and removed. These should be initialized by php_rshutdown_session_globals() always when execution is finished. */ PS(id) = NULL; PS(session_status) = php_session_none; + PS(in_save_handler) = 0; PS(mod_data) = NULL; PS(mod_user_is_open) = 0; PS(define_sid) = 1; @@ -2035,7 +2036,7 @@ static PHP_FUNCTION(session_create_id) } } - if (PS(session_status) == php_session_active) { + if (!PS(in_save_handler) && PS(session_status) == php_session_active) { int limit = 3; while (limit--) { new_id = PS(mod)->s_create_sid(&PS(mod_data)); @@ -2565,9 +2566,11 @@ static PHP_RSHUTDOWN_FUNCTION(session) /* {{{ */ { int i; - zend_try { - php_session_flush(1); - } zend_end_try(); + if (PS(session_status) == php_session_active) { + zend_try { + php_session_flush(1); + } zend_end_try(); + } php_rshutdown_session_globals(); /* this should NOT be done in php_rshutdown_session_globals() */ diff --git a/ext/session/tests/bug60634.phpt b/ext/session/tests/bug60634.phpt index b2f50762872..34786406cb1 100644 --- a/ext/session/tests/bug60634.phpt +++ b/ext/session/tests/bug60634.phpt @@ -40,16 +40,19 @@ session_write_close(); echo "um, hi\n"; /* -FIXME: Since session module try to write/close session data in -RSHUTDOWN, write() is executed twices. This is caused by undefined -function error and zend_bailout(). Current session module codes -depends on this behavior. These codes should be modified to remove -multiple write(). -*/ + * write() calls die(). This results in calling session_flush() which calls + * write() in request shutdown. The code is inside save handler still and + * calls save close handler. + * + * Because session_write_close() fails by die(), write() is called twice. + * close() is still called at request shutdown since session is active. + */ ?> --EXPECTF-- write: goodbye cruel world -write: goodbye cruel world -close: goodbye cruel world +Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 + +Warning: Unknown: Failed to write session data using user defined save handler. (session.save_path: ) in Unknown on line 0 +close: goodbye cruel world diff --git a/ext/session/tests/bug60634_error_3.phpt b/ext/session/tests/bug60634_error_3.phpt index b7840b04f9d..b38899b8c12 100644 --- a/ext/session/tests/bug60634_error_3.phpt +++ b/ext/session/tests/bug60634_error_3.phpt @@ -48,4 +48,5 @@ Stack trace: #0 [internal function]: write(%s, '') #1 {main} thrown in %s on line %d -close: goodbye cruel world + +Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 diff --git a/ext/session/tests/bug60634_error_4.phpt b/ext/session/tests/bug60634_error_4.phpt index 7970b35c7a8..3ac62479747 100644 --- a/ext/session/tests/bug60634_error_4.phpt +++ b/ext/session/tests/bug60634_error_4.phpt @@ -48,5 +48,5 @@ Stack trace: #0 [internal function]: write('%s', '') #1 {main} thrown in %s on line %d -close: goodbye cruel world +Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 diff --git a/ext/session/tests/bug69111.phpt b/ext/session/tests/bug69111.phpt index c6d10c74a0e..38201ef64a5 100644 --- a/ext/session/tests/bug69111.phpt +++ b/ext/session/tests/bug69111.phpt @@ -1,7 +1,5 @@ --TEST-- Bug #69111 Crash in SessionHandler::read() ---XFAIL-- -It is still a leak --SKIPIF-- --FILE-- @@ -19,4 +17,9 @@ $sh->write("foo", "bar"); var_dump($sh->read(@$id)); ?> --EXPECTF-- +Warning: SessionHandler::open(): Session is not active in /home/yohgaki/workspace/ext/git/oss/php.net/github-php-src/ext/session/tests/bug69111.php on line 10 + +Warning: SessionHandler::write(): Session is not active in /home/yohgaki/workspace/ext/git/oss/php.net/github-php-src/ext/session/tests/bug69111.php on line 11 + +Warning: SessionHandler::read(): Session is not active in /home/yohgaki/workspace/ext/git/oss/php.net/github-php-src/ext/session/tests/bug69111.php on line 12 bool(false) diff --git a/ext/session/tests/bug71162.phpt b/ext/session/tests/bug71162.phpt index fe38ea2999c..722889d417a 100644 --- a/ext/session/tests/bug71162.phpt +++ b/ext/session/tests/bug71162.phpt @@ -2,12 +2,12 @@ updateTimestamp never called when session data is empty --INI-- session.use_strict_mode=0 +session.save_handler=files --XFAIL-- -Current session module is designed to write empty session always. In addition, -current session module only supports SessionHandlerInterface only from PHP 7.0. +Current session module is designed to write empty session always. In addition, current session module only supports SessionHandlerInterface only from PHP 7.0. --FILE-- Date: Tue, 15 Nov 2016 12:01:13 +0900 Subject: [PATCH 6/8] Fix test --- ext/session/tests/bug69111.phpt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ext/session/tests/bug69111.phpt b/ext/session/tests/bug69111.phpt index 38201ef64a5..ce14dc750c6 100644 --- a/ext/session/tests/bug69111.phpt +++ b/ext/session/tests/bug69111.phpt @@ -17,9 +17,9 @@ $sh->write("foo", "bar"); var_dump($sh->read(@$id)); ?> --EXPECTF-- -Warning: SessionHandler::open(): Session is not active in /home/yohgaki/workspace/ext/git/oss/php.net/github-php-src/ext/session/tests/bug69111.php on line 10 +Warning: SessionHandler::open(): Session is not active in %s on line 10 -Warning: SessionHandler::write(): Session is not active in /home/yohgaki/workspace/ext/git/oss/php.net/github-php-src/ext/session/tests/bug69111.php on line 11 +Warning: SessionHandler::write(): Session is not active in %s on line 11 -Warning: SessionHandler::read(): Session is not active in /home/yohgaki/workspace/ext/git/oss/php.net/github-php-src/ext/session/tests/bug69111.php on line 12 +Warning: SessionHandler::read(): Session is not active in %s on line 12 bool(false) From d990c54c83e7c06c142344200f1e12cdfa5c8ae7 Mon Sep 17 00:00:00 2001 From: Yasuo Ohgaki Date: Tue, 15 Nov 2016 12:10:20 +0900 Subject: [PATCH 7/8] Improve error message --- ext/session/mod_user.c | 2 +- ext/session/tests/bug60634.phpt | 2 +- ext/session/tests/bug60634_error_3.phpt | 2 +- ext/session/tests/bug60634_error_4.phpt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c index 9755f0d5c49..992b0edd371 100644 --- a/ext/session/mod_user.c +++ b/ext/session/mod_user.c @@ -33,7 +33,7 @@ static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval) if (PS(in_save_handler)) { PS(in_save_handler) = 0; ZVAL_UNDEF(retval); - php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner"); + php_error_docref(NULL, E_WARNING, "Cannot call session save handler in a recursive manner"); return; } PS(in_save_handler) = 1; diff --git a/ext/session/tests/bug60634.phpt b/ext/session/tests/bug60634.phpt index 34786406cb1..a72ef07e904 100644 --- a/ext/session/tests/bug60634.phpt +++ b/ext/session/tests/bug60634.phpt @@ -52,7 +52,7 @@ echo "um, hi\n"; --EXPECTF-- write: goodbye cruel world -Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 +Warning: Unknown: Cannot call session save handler in a recursive manner in Unknown on line 0 Warning: Unknown: Failed to write session data using user defined save handler. (session.save_path: ) in Unknown on line 0 close: goodbye cruel world diff --git a/ext/session/tests/bug60634_error_3.phpt b/ext/session/tests/bug60634_error_3.phpt index b38899b8c12..776a7afea69 100644 --- a/ext/session/tests/bug60634_error_3.phpt +++ b/ext/session/tests/bug60634_error_3.phpt @@ -49,4 +49,4 @@ Stack trace: #1 {main} thrown in %s on line %d -Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 +Warning: Unknown: Cannot call session save handler in a recursive manner in Unknown on line 0 diff --git a/ext/session/tests/bug60634_error_4.phpt b/ext/session/tests/bug60634_error_4.phpt index 3ac62479747..0fd7db3ada9 100644 --- a/ext/session/tests/bug60634_error_4.phpt +++ b/ext/session/tests/bug60634_error_4.phpt @@ -49,4 +49,4 @@ Stack trace: #1 {main} thrown in %s on line %d -Warning: Unknown: Cannot call save handler function recursive manner in Unknown on line 0 +Warning: Unknown: Cannot call session save handler in a recursive manner in Unknown on line 0 From 0966c98eef10fdb89fb306257070580d927a4ee2 Mon Sep 17 00:00:00 2001 From: Joe Watkins Date: Wed, 16 Nov 2016 05:12:04 +0000 Subject: [PATCH 8/8] new entry for #2196 --- NEWS | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS b/NEWS index 86d41c56561..bdca4786725 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,9 @@ PHP NEWS - Date: . Fixed bug #69587 (DateInterval properties and isset). (jhdxr) +- Session: + . Fixed bug #73461 (Prohibit session save handler recursion) . (yohgaki) + - SQLite3: . Update to SQLite 3.15.1. (cmb)