Bug #127131 for Net-SSLeay: Memory leak from SSL_CTX_set_tlsext_status

Mon Sep 17 13:49:25 2018 leonerd-cpan [...] leonerd.org.uk - Ticket created

Subject:

Memory leak from SSL_CTX_set_tlsext_status_cb

After a lot of memory debugging (*) I have found the culprit of another memory leak. Per iteration of my program, the following line leaks three `undef` SVs, from the call to newSVsv(): SSLeay.xs line 6453: cb_data_advanced_put(ctx, "tlsext_status_cb!!data", newSVsv(data)); this appears in the SSL_CTX_set_tlsext_status_cb XS function. *: I can explain my methodology if it'll help, but it involves custom-edited perl source code, libgcc trickery, and gdb in order to print the C-level stacktrace each time newSVsv is called, and then lots of file analysis around Devel::MAT to find SV addresses of leaked SVs to pair up with the above logs. It's a long tale ;) -- Paul Evans

Tue Sep 18 11:11:38 2018 RADIATOR [...] cpan.org - Correspondence added

On ma 17.Sep 2018 13:49:25, PEVANS wrote: Show quoted text

> Per iteration of my program, the following line leaks three `undef` > SVs, from the call to newSVsv(): > > SSLeay.xs line 6453: > cb_data_advanced_put(ctx, "tlsext_status_cb!!data", > newSVsv(data)); > > this appears in the SSL_CTX_set_tlsext_status_cb XS function.

Does each iteration reset the callback when it's done with it? https://metacpan.org/pod/release/MIKEM/Net-SSLeay-1.85/lib/Net/SSLeay.pm#Callbacks The doc says that it should be done. Looking at the code, it seems like whatever values was there when callback is set, is simply overwritten as opposed to being freed first. Or does it look like the leak is related to tlsext_status_cb itself instead of how callbacks work in general? -- Heikki Vatiainen

Tue Sep 18 11:11:39 2018 The RT System itself - Status changed from 'new' to 'open'

Mon Sep 24 11:43:19 2018 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Tue Sep 18 11:11:38 2018, RADIATOR wrote: Show quoted text

> Or does it look like the leak is related to tlsext_status_cb itself > instead of how callbacks > work in general?

Looking over the source code, I don't see where the SVs stored in these fields would be freed. The string "tlsext_status_cb!!data" only appears in 3 places - 2 in the function named above which fills the fields in, and 1 in the invoke function, which fetches the value to call it. These fields appear to be part of a general SSL_CTX, so maybe they should be fetched & destroyed as part of general cleanup of a context struct? -- Paul Evans

Mon Sep 24 12:18:46 2018 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Ah. I believe I have it. The CB data is all stored in a hash, which gets unrefed by cb_advanced_data_drop(), which frees all the values stored in the hash. In cb_advanced_data_put(), we only store real SVs with defined values (where SvOK() is true): hv_delete(L2HV, data_name, strlen(data_name), G_DISCARD); if (data!=NULL) if (SvOK(data)) hv_store(L2HV, data_name, strlen(data_name), data, 0); However, in the case that the original data was the perl undef value, data here will be an SV for which SvOK() is false. That means it doesn't get stored in the hash, so the drop function won't get around to releasing it. The attached patch fixes this by SvREFCNT_dec()ing instead of hv_store()ing when the SV is undef. -- Paul Evans

Subject:

rt127131.patch

--- ../Net-SSLeay-1.85-1/SSLeay.xs 2018-01-27 20:43:03.000000000 +0000 +++ SSLeay.xs 2018-09-24 17:17:52.147889454 +0100 @@ -520,9 +520,13 @@ /* first delete already stored value */ hv_delete(L2HV, data_name, strlen(data_name), G_DISCARD); - if (data!=NULL) + if (data!=NULL) { if (SvOK(data)) hv_store(L2HV, data_name, strlen(data_name), data, 0); + else + /* we're not storing data so discard it */ + SvREFCNT_dec(data); + } return 1; }

Wed Sep 26 13:33:33 2018 RADIATOR [...] cpan.org - Correspondence added

On Mon Sep 24 12:18:46 2018, PEVANS wrote: Show quoted text

> The attached patch fixes this by SvREFCNT_dec()ing instead of > hv_store()ing when the SV is undef.

Thanks! It has now been committed to master on GitHub and will be part of the next Net::SSLeay release. -- Heikki Vatiainen

Wed Sep 26 13:33:34 2018 RADIATOR [...] cpan.org - Status changed from 'open' to 'patched'

Sun Dec 16 11:29:41 2018 chrisn [...] cpan.org - Status changed from 'patched' to 'resolved'

Sun Dec 16 11:29:42 2018 chrisn [...] cpan.org - Given to RADIATOR

Sun Dec 16 11:29:42 2018 chrisn [...] cpan.org - Fixed in 1.86_06 added

Bug #127131 for Net-SSLeay: Memory leak from SSL_CTX_set_tlsext_status_cb