-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ext/oci8: Decrease the reference count before calling zend_list_close()
#18957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: PHP-8.3
Are you sure you want to change the base?
Conversation
Thank you a lot for fixing this! ❤ |
I have cherrypicked this into unbundled oci8 ext - php/pecl-database-oci8#32 - and I can confirm this fixes the issue/test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, one minor suggestion.
Also: don't we want a test for this? Not that I care too much about this (oracle didn't given the nr of open bugs, so why would I?) but would still be nice I presume.
ext/oci8/oci8.c
Outdated
GC_DELREF(column->descid); | ||
} | ||
if (column->descid && !GC_DELREF(column->descid)) { | ||
zend_list_close(column->descid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the original code wanted to call zend_list_delete
instead of zend_list_close
.
Your fix is fine, but can be improved further by using zend_list_free
instead of zend_list_close
as we already know the refcount is 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are at least 5 other zend_list_close
https://github.com/search?q=repo%3Aphp%2Fpecl-database-oci8%20zend_list_close&type=code calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That code is different.
Thanks! I’ll change it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test won't work when ZendMM is disabled, so perhaps it should be skipped in that case but it doesn't hurt running it anyway.
Patch still LGTM
@@ -5,6 +5,14 @@ oci8 | |||
--SKIPIF-- | |||
<?php | |||
require_once 'skipifconnectfailure.inc'; | |||
|
|||
ob_start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an easier way, see
php-src/tests/output/gh15179.phpt
Line 9 in 4a98b36
if (getenv("USE_ZEND_ALLOC") === "0") die("skip requires ZendMM"); |
fixes #18873
Since the resource will eventually be freed at the end of the script, this does not result in an actual memory leak, but this change fixes the issue of unnecessary memory usage growth.