-
Notifications
You must be signed in to change notification settings - Fork 8k
pgsql: Fix memory leak when object init fails #20387
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
Conversation
The return value is already overwritten by this point so we do have to clean up the old return value (i.e. dataset) after all.
| ZVAL_COPY_VALUE(&dataset, return_value); | ||
| zend_result obj_initialized = object_init_ex(return_value, ce); | ||
| if (UNEXPECTED(obj_initialized == FAILURE)) { | ||
| zval_ptr_dtor(&dataset); |
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.
I might confuse but you said to girgias in the other PR
In fact, zval_ptr_dtor(&dataset); should also be dropped as dataset is a borrow of the return value.
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.
Here.
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.
Yeah I was wrong about that. I missed that object_init_ex overwrites the return value also in case of failure
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.
ah true true I see it now.
Both enums and integers map to TidyInteger, however, in the TidyInteger case we always used zval_get_long(). So for a non-numeric string, this would get turned into 0. 0 is the first enum value in that case, so the wrong enum value could be selected. To solve this, add special handling for strings and (stringable) objects such that we can explicitly check for numeric strings, and if they're not, handle them as normal strings instead of as 0. Closes GH-20387.
The return value is already overwritten by this point so we do have to clean up the old return value (i.e. dataset) after all.