Skip to content

Conversation

chuanma
Copy link

@chuanma chuanma commented Mar 25, 2014

Related to the bug https://bugs.php.net/bug.php?id=66942 I reported a couple of days ago.

A similar memory leak in openssl_encrypt() (https://bugs.php.net/bug.php?id=54060) has been fixed by calling EVP_CIPHER_CTX_cleanup() once. But here we need to call it twice because the variable ctx is reused.

Also fixed the memory leak in openssl_open(). https://bugs.php.net/bug.php?id=66952

@chuanma chuanma changed the title Fix #66942: openssl_seal() memory leak Fix #66942 #66952: openssl_seal(), openssl_open() memory leak Mar 25, 2014
@bukka
Copy link
Member

bukka commented Mar 25, 2014

Hi, it fixes same mem leaks but not all. The openssl_open need to be changed to address all cases. There are RETURN_FALSE that can result in memory leak. It should be changed to RETVAL_FALSE and then do the final clean up. Look to the openssl_encrypt. It would be probably best to do it similar way...

@chuanma
Copy link
Author

chuanma commented Mar 25, 2014

Thanks for the suggesiton. I'll clean up that section of code a bit more to cover all cases.

@chuanma
Copy link
Author

chuanma commented Mar 26, 2014

replace RETURN_FALSE by RETVAL_FALSE, RETURN_TURE by RETVAL_TRUE in a few places such that I can move cleanup code to the end of the function, and remove a couple of duplicate if blocks as well.

@smalyshev
Copy link
Contributor

I see in the tests this:

TEST 5193/12224 [ext/openssl/tests/002.phpt]
========DIFF========
002+ Segmentation fault

Which is not good. Looks like the patch is not right.

@chuanma
Copy link
Author

chuanma commented Apr 14, 2014

@smalyshev Thanks for pointing that out. I missed one corner case. I'll get a fix soon and pay more attention to the Travis CI build.

…_seal() because in one corner case the variable ctx has not been initiated yet.
@chuanma
Copy link
Author

chuanma commented Apr 14, 2014

Ok. My previous commit fixed that test in [ext/openssl/tests/002.phpt]. Travis CI build failed from other reasons.

Because the normal workflow is already tested in [ext/openssl/tests/013.phpt], I don't add a new test case.

@php-pulls
Copy link

Comment on behalf of stas at php.net:

merged

@php-pulls php-pulls closed this Apr 20, 2014
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