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

ext/session/mod_user.c

@@ -17,6 +17,7 @@
 #include "php.h"
 #include "php_session.h"
 #include "mod_user.h"
+#include "zend_exceptions.h"
 
 const ps_module ps_mod_user = {
 	PS_MOD_UPDATE_TIMESTAMP(user)
@@ -112,6 +113,7 @@ PS_CLOSE_FUNC(user)
 	bool bailout = 0;
 	zval retval;
 	zend_result ret = FAILURE;
+	zend_object *old_exception = NULL;
 
 	ZEND_ASSERT(!Z_ISUNDEF(PSF(close)));
 
@@ -120,6 +122,12 @@ PS_CLOSE_FUNC(user)
 		return SUCCESS;
 	}
 
+	/* Run close() even with a pending exception, so the handler releases its resources; skip real exit()/die() (bug #60634). */
+	if (EG(exception) && !zend_is_unwind_exit(EG(exception)) && !zend_is_graceful_exit(EG(exception))) {
+		old_exception = EG(exception);
+		EG(exception) = NULL;
+	}
+
 	zend_try {
 		ps_call_handler(&PSF(close), 0, NULL, &retval);
 	} zend_catch {
@@ -128,6 +136,17 @@ PS_CLOSE_FUNC(user)
 
 	PS(mod_user_implemented) = 0;
 
+	if (old_exception) {
+		if (!EG(exception)) {
+			EG(exception) = old_exception;
+		} else if (!zend_is_unwind_exit(EG(exception)) && !zend_is_graceful_exit(EG(exception))) {
+			zend_exception_set_previous(EG(exception), old_exception);
+		} else {
+			/* close() itself exited/died: that takes precedence, the stashed exception is moot. */
+			OBJ_RELEASE(old_exception);
+		}
+	}
+
 	if (bailout) {
 		if (!Z_ISUNDEF(retval)) {
 			zval_ptr_dtor(&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,53 @@
+--TEST--
+GH-16027 (close() throwing while a write() exception is pending chains it as previous)
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+class MySessionHandler implements SessionHandlerInterface {
+    function open($save_path, $session_name): bool {
+        return true;
+    }
+
+    function close(): bool {
+        echo "close: goodbye cruel world\n";
+        throw new Exception('close failed');
+    }
+
+    function read($id): string|false {
+        return '';
+    }
+
+    function write($id, $session_data): bool {
+        echo "write: goodbye cruel world\n";
+        throw new Exception('write failed');
+    }
+
+    function destroy($id): bool {
+        return true;
+    }
+
+    function gc($maxlifetime): int {
+        return 1;
+    }
+}
+
+session_set_save_handler(new MySessionHandler());
+session_start();
+
+try {
+    session_write_close();
+} catch (\Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), "\n";
+    $previous = $e->getPrevious();
+    echo 'previous: ';
+    var_dump($previous ? $previous->getMessage() : null);
+}
+?>
+--EXPECT--
+write: goodbye cruel world
+close: goodbye cruel world
+Exception: close failed
+previous: string(12) "write failed"

ext/session/tests/gh16027_3.phpt

@@ -0,0 +1,51 @@
+--TEST--
+GH-16027 (exit() in close() while a write() exception is pending terminates cleanly)
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+class MySessionHandler implements SessionHandlerInterface {
+    function open($save_path, $session_name): bool {
+        return true;
+    }
+
+    function close(): bool {
+        echo "close: goodbye cruel world\n";
+        exit("bye\n");
+    }
+
+    function read($id): string|false {
+        return '';
+    }
+
+    function write($id, $session_data): bool {
+        echo "write: goodbye cruel world\n";
+        throw new Exception('write failed');
+    }
+
+    function destroy($id): bool {
+        return true;
+    }
+
+    function gc($maxlifetime): int {
+        return 1;
+    }
+}
+
+session_set_save_handler(new MySessionHandler());
+session_start();
+
+try {
+    session_write_close();
+    echo "unreachable\n";
+} catch (\Throwable $e) {
+    echo "unreachable catch: ", $e->getMessage(), "\n";
+}
+echo "unreachable after\n";
+?>
+--EXPECT--
+write: goodbye cruel world
+close: goodbye cruel world
+bye

ext/session/tests/user_session_module/bug60634_error_1.phpt

@@ -43,14 +43,10 @@ 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
+close: goodbye cruel world
 
 Fatal error: Uncaught Error: Call to undefined function undefined_function() in %s:%d
 Stack trace:

ext/session/tests/user_session_module/bug60634_error_2.phpt

@@ -47,6 +47,7 @@ echo "um, hi\n";
 ?>
 --EXPECTF--
 write: goodbye cruel world
+close: goodbye cruel world
 
 Fatal error: Uncaught Exception in %s
 Stack trace:

ext/session/tests/user_session_module/session_set_save_handler_class_012.phpt

@@ -43,6 +43,8 @@ var_dump(session_id(), $oldHandler, ini_get('session.save_handler'), $handler->i
 --EXPECTF--
 *** Testing session_set_save_handler() : incorrect arguments for existing handler open ***
 Open:
+
+Warning: SessionHandler::close(): Parent session handler is not open in %s on line %d
 SessionHandler::open() expects exactly 2 arguments, 0 given
 
 Warning: Undefined global variable $_SESSION in %s on line %d