Skip to content

Bugfix 72791: fix memory leak in PDO persistent connections #2067

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

Merged
merged 2 commits into from
Aug 9, 2016

Conversation

keyurdg
Copy link
Contributor

@keyurdg keyurdg commented Aug 9, 2016

Attempt 2. This supersedes PR #2066

@keyurdg
Copy link
Contributor Author

keyurdg commented Aug 9, 2016

@nikic This is a real leak, see valgrind output here: https://bugs.php.net/bug.php?id=72791

@nikic
Copy link
Member

nikic commented Aug 9, 2016

@keyurdg I realize that it's a real leak, the question is more whether this is a "real" fix, or whether it effectively only suppresses the valgrind warning. I'm asking because I don't fully understand the reason why it's necessary to delay this free to the end of the request. Doesn't this indicate that refcounting is going wrong somewhere else?

@nikic
Copy link
Member

nikic commented Aug 9, 2016

Ah wait, I didn't see that this PR no longer delays the destruction, so that comment is no longer relevant.

@keyurdg
Copy link
Contributor Author

keyurdg commented Aug 9, 2016

@nikic just to clarify, the old PR actually free'd the memory. Without a list to save pointer references in, all references to the older pecalloc'd pdbh were lost when it was updated in the EG(persistent_list). zend_hash_str_update_mem() free's the enclosing zend_resource object (php_pdo_list_entry), but doesn't actually free the pointer saved inside it.

I do take the point that within a request the memory was not free'd as soon as feasible.

In any case, no longer relevant :)

@rlerdorf
Copy link
Member

rlerdorf commented Aug 9, 2016

This looks correct to me

@php-pulls php-pulls merged commit 2ab9a2d into php:PHP-7.0 Aug 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants