Page MenuHomePhabricator

emile: fix a potentional resource leaking.
ClosedPublic

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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.Mar 16 2020, 9:51 PM

rebasing

@bu5hm4n, @ProhtMeyhet are you happy with the rebased version now?

@bu5hm4n, @ProhtMeyhet are you happy with the rebased version now?

I was just answering a question. So let me quote my favourite actor: "No comment".

(sorry, this self quarantine needs jokes)

bu5hm4n accepted this revision.Apr 5 2020, 10:44 PM

@bu5hm4n, @ProhtMeyhet are you happy with the rebased version now?

I was just answering a question. So let me quote my favourite actor: "No comment".

(sorry, this self quarantine needs jokes)

+1

This revision is now accepted and ready to land.Apr 5 2020, 10:44 PM
Closed by commit rEFL3cf0bdd599d7: emile: fix a potentional resource leaking. (authored by WhiskyKiloSq, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyApr 6 2020, 12:05 AM
This revision was automatically updated to reflect the committed changes.