openssl / openssl

TLS/SSL and crypto library
https://www.openssl.org
Apache License 2.0
24.84k stars 9.92k forks source link

memory leak in OPENSSL_config #24729

Closed bernd-edlinger closed 1 day ago

bernd-edlinger commented 3 days ago

The function OPENSSL_config leaks memory:

https://github.com/openssl/openssl/blob/42a8ef844e5fca55abb608beb62695abe80c6b6d/crypto/conf/conf_sap.c#L34-L41

we should add a statement like

   free (settings.appname);
Sashan commented 3 days ago

The function OPENSSL_config leaks memory:

https://github.com/openssl/openssl/blob/42a8ef844e5fca55abb608beb62695abe80c6b6d/crypto/conf/conf_sap.c#L34-L41

we should add a statement like

   free (settings.appname);

But what's the point doing a strdup() here. I think it makes more sense to replace the call to strdup() with plain assingment it should have the same effect.

t8m commented 3 days ago

But what's the point doing a strdup() here. I think it makes more sense to replace the call to strdup() with plain assingment it should have the same effect.

Hmm... that's a good question. I assume the code was prepared for a delayed initialization where settings was saved somewhere and used later. However we AFAIK do not do it.

nhorman commented 3 days ago

we don't save the appname off somewhere down the call path do we? I looked, but didn't find anything, unless providers somehow have the opportunity to do so

bernd-edlinger commented 3 days ago

But what's the point doing a strdup() here. I think it makes more sense to replace the call to strdup() with plain assingment it should have the same effect.

I think a direct assignment of the input parameter will receive a warning because of the constness of the input parameter, so instead of the strdup an explicit type cast would be necessary but that looks more ugly than a strdup IMHO.

nhorman commented 2 days ago

ack @bernd-edlinger Its not a hot path either allocating and freeing isn't going to hurt anything here