Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

On Tue, Jul 12, 2016 at 05:25:55PM -0700, Roberto Guimaraes wrote: > It???s a double-free???. we need to NULL local_dh_1024 after the free because > your code has the following destructor (1.5 doesn???t): > > __attribute__((destructor)) > static void __ssl_sock_deinit(void) > { > #ifdef

Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

On Tue, Jul 12, 2016 at 02:55:58PM -0700, Roberto Guimaraes wrote: > I will certainly pick Remi???s fix, thanks! > > Did you have any issues with haproxy 1.5 by chance? No, we didn't backport the fix. The older the branch, the more critical the fixes need to be in order to be backported. > This

Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

It’s a double-free…. we need to NULL local_dh_1024 after the free because your code has the following destructor (1.5 doesn’t): __attribute__((destructor)) static void __ssl_sock_deinit(void) { #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME lru64_destroy(ssl_ctx_lru_tree); #endif #ifndef

Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

I will certainly pick Remi’s fix, thanks! Did you have any issues with haproxy 1.5 by chance? This has been running in prod (multiple boxes) for over a month without a single issue. And your example config doesn’t cause any problems either. * also using openssl 1.0.2h hmm wouldn’t valgrind

Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

On Tue, Jul 12, 2016 at 02:11:39PM -0700, Roberto Guimaraes wrote: > My apologies and thanks for the heads up! I will revert it on my end as well. > Weird, nginx seems to have the same unconditional DH_free in there. I had no problem with this on my laptop with openssl 1.0.1 and the few tests we

Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

My apologies and thanks for the heads up! I will revert it on my end as well. Weird, nginx seems to have the same unconditional DH_free in there. roberto > On Jul 12, 2016, at 2:49 AM, Willy Tarreau wrote: > > On Sat, Jul 02, 2016 at 04:47:57PM +0200, Remi Gacogne wrote: >> I've

Re: Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

On Sat, Jul 02, 2016 at 04:47:57PM +0200, Remi Gacogne wrote: > I've attached what I think is the right fix, simply checking if > local_dh_2014 is NULL before calling ssl_get_dh_1024(), thus only > generating it at most one time. It's then freed in __ssl_sock_deinit() > so valgrind should be

Re: Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

Hi Rémi, sorry for the slow reply :-( On Sat, Jul 02, 2016 at 04:47:57PM +0200, Remi Gacogne wrote: > Hi Willy, Roberto, Emeric, > > On 06/30/2016 08:12 PM, Willy Tarreau wrote: > > I checked the man page for SSL_CTX_set_tmp_dh() and it does not mention > > anything regarding the need to free

Re: Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

> Date: Sat, 2 Jul 2016 16:26:10 +0200 Subject: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params() Roberto Guimaraes reported that Valgrind complains about a leak in ssl_get_dh_1024(). This is caused caused by an oversight in ssl_sock_load_dh_params(), where lo

Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

Hi Roberto, On Sun, Jun 12, 2016 at 01:13:16PM +0200, Willy Tarreau wrote: > On Sat, Jun 11, 2016 at 03:58:10PM -0700, Roberto Guimaraes wrote: > > Valgrind reports that the memory allocated in ssl_get_dh_1024() was > > leaking. Upon further inspection of openssl code, it seems that > >

Re: [PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

On Sat, Jun 11, 2016 at 03:58:10PM -0700, Roberto Guimaraes wrote: > Valgrind reports that the memory allocated in ssl_get_dh_1024() was leaking. > Upon further inspection of openssl code, it seems that SSL_CTX_set_tmp_dh > makes a copy of the data, so calling DH_free afterwards makes sense. >

[PATCH] BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()

Valgrind reports that the memory allocated in ssl_get_dh_1024() was leaking. Upon further inspection of openssl code, it seems that SSL_CTX_set_tmp_dh makes a copy of the data, so calling DH_free afterwards makes sense. thanks, roberto --- src/ssl_sock.c | 1 + 1 file changed, 1 insertion(+)