ext/session: Fix memory leak

[arshidkv12]

Fix the memory leak.

class MySession extends SessionHandler {
    public function validateId($key) {
        return null; //memory leak here
    }
}

$handler = new MySession();

try {
    session_set_save_handler($handler);
    session_start();
} catch (TypeError $e) {
   echo $e->getMessage(), "\n";
}

session_write_close();

try {
    session_start();
} catch (Throwable $e) {
   echo $e->getMessage(), "\n";
}

[devnexen]

what memory leak ? you should probably show its output.

[arshidkv12]

Output:

sapi/cli/php z.php                                              

Fatal error: Uncaught TypeError: Session callback must have a return value of type bool, null returned in /Users/arshid/Downloads/php-src/z.php:18
Stack trace:
#0 /Users/arshid/Downloads/php-src/z.php(18): session_start()
#1 {main}

Next Error: Session id must be a string in /Users/arshid/Downloads/php-src/z.php:18
Stack trace:
#0 /Users/arshid/Downloads/php-src/z.php(18): session_start()
#1 {main}
  thrown in /Users/arshid/Downloads/php-src/z.php on line 18
[Wed Feb 11 19:18:15 2026]  Script:  '/Users/arshid/Downloads/php-src/z.php'
/Users/arshid/Downloads/php-src/Zend/zend_string.h(167) :  Freeing 0x000000010168b230 (80 bytes), script=/Users/arshid/Downloads/php-src/z.php
[Wed Feb 11 19:18:15 2026]  Script:  '/Users/arshid/Downloads/php-src/z.php'
/Users/arshid/Downloads/php-src/ext/session/mod_files.c(415) :  Freeing 0x0000000101690690 (40 bytes), script=/Users/arshid/Downloads/php-src/z.php
=== Total 2 memory leaks detected ===

[devnexen]

that seems a workaround, and probably Gina would prefer to keep as an exception but will leave it to her.

[Girgias]

Yes, just add zval_ptr_dtor(&value); also this is a bug fix and needs to be backported.

[arshidkv12]

Ok. Thank you

[arshidkv12]

Please note: Memory leak is in the PS_OPEN_FUNC(files) function.

	if (!EG(exception)) {
		zend_type_error("Session callback must have a return value of type bool, %s returned", zend_zval_value_name(value)); 
		zval_ptr_dtor(&value);
    }

Output:

sapi/cli/php z.php                                                     
Assertion failed: (zval_gc_type((ref)->gc.u.type_info) == 7 || zval_gc_type((ref)->gc.u.type_info) == 8), function gc_possible_root, file zend_gc.c, line 758.
[36]    60168 abort      sapi/cli/php z.php

[devnexen]

@Girgias seeing Zend/zend_string.h(167) : Freeing 0x000000010168b230 (80 bytes) could it be like more the id global ? if you do not have time I may look at it later.

[devnexen]

I cannot reproduce but can you try the following instead ?

if (!EG(exception)) {
    if (PS(id)) {
        zend_string_release(PS(id));
        PS(id) = NULL;
    }
    zend_type_error("Session callback must have a return value of type bool, %s returned", zend_zval_value_name(value));
}

[devnexen]

I tried with these flags -d session.save_handler=files -d session.save_path=/tmp but could not reproduce

[arshidkv12]

php.ini:

session.use_strict_mode=1
session.name=PHPSESSID
session.save_handler=files

[devnexen]

came up with this draft, do not think it is fully fix all cases tough. @Girgias thoughts ? at least the leak went away.

diff --git a/ext/session/session.c b/ext/session/session.c
index d9a4e2b5a4d..14ce69769f0 100644
--- a/ext/session/session.c
+++ b/ext/session/session.c
@@ -1725,14 +1725,22 @@ PHPAPI php_session_status php_get_session_status(void)
 
 static bool php_session_abort(void)
 {
-       if (PS(session_status) == php_session_active) {
-               if (PS(mod_data) || PS(mod_user_implemented)) {
-                       PS(mod)->s_close(&PS(mod_data));
-               }
-               PS(session_status) = php_session_none;
-               return true;
-       }
-       return false;
+       if (PS(session_status) == php_session_active) {
+           if (PS(mod_data) || PS(mod_user_implemented)) {
+               zend_object *exception = EG(exception);
+               EG(exception) = NULL;
+               PS(mod)->s_close(&PS(mod_data));
+              // if s_close callback set an exception, we set
+              // it back to the one raised earlier
+              // FIXME: would zend_exception_set_previous be better ?
+               if (exception) {
+                   EG(exception) = exception;
+               }
+           }
+           PS(session_status) = php_session_none;
+           return true;
+       }
+       return false;
 }

[devnexen]

with your case @arshidkv12 I get this after fix

sapi/cli/php a.php 
Session id must be a string

[Girgias]

CI is red for a reason.

[devnexen]

lctm

[Girgias]

Note to self (or other merger) needs to target PHP 8.4

[Girgias]

Can you please squash and rebase? As the current set of commits makes it hard to apply and merge with git am.

[arshidkv12]

@Girgias Please check it

[ndossche]

I believe the fix is wrong and should be reverted.

[ndossche]

Why do you check for PS(mod)->s_close here? It didn't happen previously and I don't see similar other checks.

[ndossche]

Temporarily swallowing the exception is a code smell. Now the behaviour changes if the userland code threw an exception.

Also this doesn't fix the issue at all...
Add the following code to the test inside the class and you have the exact same leaks:

    public function close(): bool {
        return false;
    }