ext/session: Fix GH-16027 session close() not called when write fails

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)) {

ext/session/tests/gh16027.phpt

@@ -0,0 +1,45 @@
+--TEST--
+GH-16027 (Using SessionHandler doesn't always close session file)
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+$save_path = __DIR__ . '/gh16027';
+@mkdir($save_path);
+session_save_path($save_path);
+
+$id = str_repeat('a1', 16);
+session_id($id);
+session_set_save_handler(new SessionHandler(), true);
+session_start();
+
+try {
+    $_SESSION['test'] = function () {};
+    session_write_close();
+} catch (\Throwable $e) {
+    echo $e::class, ': ', $e->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--
+<?php
+$save_path = __DIR__ . '/gh16027';
+$id = str_repeat('a1', 16);
+@unlink("$save_path/sess_$id");
+@rmdir($save_path);
+?>
+--EXPECT--
+Exception: Serialization of 'Closure' is not allowed
+session file exists: bool(true)
+lock acquired after failed write: bool(true)

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--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+$save_path = __DIR__ . '/gh16027_2';
+@mkdir($save_path);
+session_save_path($save_path);
+
+$id = str_repeat('b1', 16);
+session_id($id);
+
+class ThrowingCloseHandler extends SessionHandler {
+    function close(): bool {
+        throw new Exception('close blew up before delegating');
+    }
+}
+
+session_set_save_handler(new ThrowingCloseHandler(), true);
+session_start();
+
+try {
+    session_write_close();
+} catch (\Throwable $e) {
+    echo $e::class, ': ', $e->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--
+<?php
+$save_path = __DIR__ . '/gh16027_2';
+$id = str_repeat('b1', 16);
+@unlink("$save_path/sess_$id");
+@rmdir($save_path);
+?>
+--EXPECT--
+Exception: close blew up before delegating
+session file exists: bool(true)
+lock acquired after close() threw: bool(true)

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