ext/session: fix missing zval_ptr_dtor for retval in PS_GC_FUNC(user)#21747
ext/session: fix missing zval_ptr_dtor for retval in PS_GC_FUNC(user)#21747jorgsowa wants to merge 2 commits intophp:PHP-8.4from
Conversation
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
Needs to be backported to 8.4.
PS_GC_FUNC(user) did not call zval_ptr_dtor() on the return value of the user GC callback, leaking memory when the callback returned a reference-counted value. All other user handlers (write, destroy, validate_sid, update_timestamp) already free retval correctly.
jorgsowa
force-pushed
the
fix/session-gc-retval-leak
branch
from
1c709a2 to
2330863
Compare
| function(string $id): string|false { return ""; }, | ||
| function(string $id, string $data) { return true; }, | ||
| function(string $id) { return true; }, | ||
| function(int $max) { return str_repeat("x", 100); } |
There was a problem hiding this comment.
Just FYI: this won't leak when the test runs with opcache, which nowadays is almost always. This is because of SCCP computing the string at compile time which causes the string to get interned.
To solve this, replace 100 by random_int(100, 100)
| /* Anything else is some kind of error */ | ||
| *nrdels = -1; // Error | ||
| } | ||
| zval_ptr_dtor(&retval); |
There was a problem hiding this comment.
Why not only do this in the error case?
Besides there's a subtle additional problem in the callback code: it doesn't handle reference returns (this may be solved in ps_call_handler).
There was a problem hiding this comment.
Thank you for this comment. I need to still apply it, but in different PR. CC: @Girgias this hasn't been addressed yet, but I think it may be addressed in differnt PR
|
@jorgsowa can you address Nora's remarks? |
Use random_int(100, 100) to defeat SCCP constant folding, which would otherwise intern the str_repeat result at compile time under opcache and make the leak unobservable.
3 participants
PS_GC_FUNC(user) did not call zval_ptr_dtor() on the return value of the user GC callback, leaking memory when the callback returned a reference-counted value. All other user handlers (write, destroy, validate_sid, update_timestamp) already free retval correctly.