diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c index 71b18612683d..5f98f4169b3c 100644 --- a/ext/session/mod_user.c +++ b/ext/session/mod_user.c @@ -23,6 +23,17 @@ const ps_module ps_mod_user = { }; +/* Releases the native handler SessionHandler wraps when its close() never ran. */ +static void ps_close_default_mod(void) +{ + if (PS(mod_user_is_open) && PS(default_mod)) { + PS(mod_user_is_open) = false; + zend_try { + PS(default_mod)->s_close(&PS(mod_data)); + } zend_end_try(); + } +} + static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval) { int i; @@ -32,12 +43,17 @@ static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval) php_error_docref(NULL, E_WARNING, "Cannot call session save handler in a recursive manner"); } else { PS(in_save_handler) = 1; - if (call_user_function(NULL, NULL, func, retval, argc, argv) == FAILURE) { - zval_ptr_dtor(retval); - ZVAL_UNDEF(retval); - } else if (Z_ISUNDEF_P(retval)) { - ZVAL_NULL(retval); - } + zend_try { + if (call_user_function(NULL, NULL, func, retval, argc, argv) == FAILURE) { + zval_ptr_dtor(retval); + ZVAL_UNDEF(retval); + } else if (Z_ISUNDEF_P(retval)) { + ZVAL_NULL(retval); + } + } zend_catch { + ps_close_default_mod(); + zend_bailout(); + } zend_end_try(); PS(in_save_handler) = 0; } for (i = 0; i < argc; i++) { @@ -75,8 +91,8 @@ static zend_result verify_bool_return_type_userland_calls(const zval* value) } if (!EG(exception)) { zend_type_error("Session callback must have a return value of type bool, %s returned", zend_zval_value_name(value)); \ - } - return FAILURE; + } + return FAILURE; } PS_OPEN_FUNC(user) @@ -127,6 +143,7 @@ PS_CLOSE_FUNC(user) } zend_end_try(); PS(mod_user_implemented) = 0; + ps_close_default_mod(); if (bailout) { if (!Z_ISUNDEF(retval)) { diff --git a/ext/session/tests/gh16027.phpt b/ext/session/tests/gh16027.phpt new file mode 100644 index 000000000000..40a77f573af1 --- /dev/null +++ b/ext/session/tests/gh16027.phpt @@ -0,0 +1,45 @@ +--TEST-- +GH-16027 (Using SessionHandler doesn't always close session file) +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- +getMessage(), "\n"; +} + +$file = "$save_path/sess_$id"; +echo "session file exists: "; +var_dump(file_exists($file)); + +/* A leaked lock from close() never being called would make this fail immediately. */ +$fp = fopen($file, 'r+'); +echo "lock acquired after failed write: "; +var_dump(flock($fp, LOCK_EX | LOCK_NB)); +fclose($fp); +?> +--CLEAN-- + +--EXPECT-- +Exception: Serialization of 'Closure' is not allowed +session file exists: bool(true) +lock acquired after failed write: bool(true) diff --git a/ext/session/tests/gh16027_2.phpt b/ext/session/tests/gh16027_2.phpt new file mode 100644 index 000000000000..223485005bdd --- /dev/null +++ b/ext/session/tests/gh16027_2.phpt @@ -0,0 +1,51 @@ +--TEST-- +GH-16027 (SessionHandler subclass throwing inside close() still releases the session file lock) +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- +getMessage(), "\n"; +} + +$file = "$save_path/sess_$id"; +echo "session file exists: "; +var_dump(file_exists($file)); + +/* close() threw before ever calling parent::close(); the lock must still be released. */ +$fp = fopen($file, 'r+'); +echo "lock acquired after close() threw: "; +var_dump(flock($fp, LOCK_EX | LOCK_NB)); +fclose($fp); +?> +--CLEAN-- + +--EXPECT-- +Exception: close blew up before delegating +session file exists: bool(true) +lock acquired after close() threw: bool(true) diff --git a/ext/session/tests/user_session_module/bug60634_error_1.phpt b/ext/session/tests/user_session_module/bug60634_error_1.phpt index 98e4d371dd8c..d27d2d34b37d 100644 --- a/ext/session/tests/user_session_module/bug60634_error_1.phpt +++ b/ext/session/tests/user_session_module/bug60634_error_1.phpt @@ -43,11 +43,6 @@ session_start(); session_write_close(); echo "um, hi\n"; -/* -FIXME: Something wrong. It should try to close after error, otherwise session -may keep "open" state. -*/ - ?> --EXPECTF-- write: goodbye cruel world