Page MenuHomePhabricator

emile: fix a potentional resource leaking.
Needs ReviewPublic

Authored by akanad on Feb 17 2020, 4:38 AM.

Details

Summary

'ctx' can be leaked in some case.
this pathc fixes it.

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D11368
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16338
Build 10952: arc lint + arc unit
akanad created this revision.Feb 17 2020, 4:38 AM
akanad requested review of this revision.Feb 17 2020, 4:38 AM

@cedric can you have a look here, i am not sure if we need to call cleanup *and* free or only one of it.

bu5hm4n requested changes to this revision.Feb 19 2020, 6:06 AM

Okay, this new line should go into the #else cause of the OPENSSL_VERSION_NUMBER, otherwise you call free after cleanup, however, cleanup also frees all internals, but not the pointer, hence i am not sure if this wouldn't crash, or result in undefined behavior.

This revision now requires changes to proceed.Feb 19 2020, 6:06 AM

in the github link, a patch looks like below.

+#if OPENSSL_VERSION_NUMBER >= 0x1010100fL
+ EVP_CIPHER_CTX_cleanup(ctx);
+ EVP_CIPHER_CTX_free(ctx);
+#else
+ EVP_CIPHER_CTX_cleanup(&ctx);
+#endif

I wonder a version when behavior of those apis has changed.
github code indicates 0x1010100f as version. and 0x10100000 at efl.

I roughly went through changelog on openssl. no luck.

Okay, this now at least does not cause a version problem. However, i think the cleanup call in here is not needed as you can see in https://github.com/openssl/openssl/blob/master/crypto/evp/evp_enc.c#L66 that reset is called when free is called. (reset is also called when cleanup is called. However, i think this is now better than before, i will either land this, or the "only free call" later today.

ProhtMeyhet added subscribers: raster, ProhtMeyhet.EditedFeb 20 2020, 7:47 PM

in the github link, a patch looks like below.

please use the code function here in the future. Thanks :-)

I wonder a version when behavior of those apis has changed.
github code indicates 0x1010100f as version. and 0x10100000 at efl.

I roughly went through changelog on openssl. no luck.

This question is ambiguous. The api of OpenSSL changed in 1.1. The moreover problem is that LibreSSL for whatever reason also now uses to use the constant OPENSSL_VERSION_NUMBER, but for it's own purpose... thats why the check || !defined(LIBRESSL_VERSION_NUMBER) is in efl. *shrugs*

There is more explanation in T4746 for changes in efl. (Be sure to check Show Older Changes changes link)

akanad updated this revision to Diff 29521.Mon, Mar 16, 9:51 PM

rebasing