-
Notifications
You must be signed in to change notification settings - Fork 8k
pgsql: Fix memory leak when first string conversion fails #20213
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
If the first string conversion fails, then i==0, but memory was still allocated for `params`. However, we skip freeing `params` when i==0.
} | ||
efree(params); | ||
} | ||
efree(params); |
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.
was it the "point of contention" ? in that case you can just change it place.
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 don't understand what you mean
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.
if num_params == 0 then params array was never freed
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.
Yes, e.g. if you exit here in the first iteration when i==0, then params
is never freed:
Lines 1130 to 1131 in 28ce1b0
_php_pgsql_free_params(params, i); | |
RETURN_THROWS(); |
Removing the num_params > 0
check fixes it. It is a useless check anyway: the loop will already check this and we should not do the check for freeing params
.
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.
sure what I meant originally was just putting the efree(params);
out of the if block would have worked as well but that s detail.
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 see. That works too but since I'm touching the function anyway I thought I'd just remove the check
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.
Yes I just also saw your other PR, makes sense
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.
anyhow as long the fix is correct...
If the first string conversion fails, then i==0, but memory was still allocated for
params
. However, we skip freeingparams
when i==0.