Skip Menu |
 

This queue is for tickets about the Net-SSLeay CPAN distribution.

Report information
The Basics
Id: 127131
Status: patched
Priority: 0/
Queue: Net-SSLeay

People
Owner: Nobody in particular
Requestors: leonerd-cpan [...] leonerd.org.uk
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: 1.85
Fixed in: (no value)



Subject: Memory leak from SSL_CTX_set_tlsext_status_cb
Download (untitled) / with headers
text/plain 689b
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
Download (untitled) / with headers
text/plain 781b
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
Download (untitled) / with headers
text/plain 606b
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
Download (untitled) / with headers
text/plain 764b
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
Download rt127131.patch
text/x-diff 519b
--- ../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; }
Download (untitled) / with headers
text/plain 273b
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


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.