Bug 994 - Bindings are leaking
Summary: Bindings are leaking
Status: CONFIRMED
Alias: None
Product: libsigrok
Classification: Unclassified
Component: Bindings: C++ (show other bugs)
Version: unreleased development snapshot
Hardware: All All
: Normal normal
Target Milestone: ---
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-05 20:49 CEST by Soeren Apel
Modified: 2018-09-09 11:26 CEST (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Soeren Apel 2017-07-05 20:49:49 CEST
http://sigrok.org/gitweb/?p=libsigrok.git;a=blob;f=bindings/cxx/classes.cpp#l261

g_new() and friends are called in several places but there is no corresponding g_free() call anywhere. It was assumed that sr_packet_free() would be called but this turned out to be false. With that, the memory allocated by the bindings is never freed.
Comment 1 Martin Ling 2018-09-09 11:26:02 CEST
I have just been having a look at this. A few notes:

1. The methods that return leaving memory allocated by g_new/g_malloc are:

 - Context::create_header_packet()
 - Context::create_meta_packet()
 - Context::create_logic_packet()
 - Context::create_analog_packet()
 - Analog::get_logic_via_threshold()
 - Analog::get_logic_via_schmitt_trigger()

These are all places where we create a new Packet/Payload at the user's request, so it's owned by the user, and should be freed when its shared_ptr refcount hits zero (but currently isn't).

2. As far as I can see we don't leak anything in the common case, where Packet objects are created from structures passed into the datafeed callback by a driver or input module.

3. The obvious solution of just putting the corresponding frees in the destructors isn't right, because we would then be trying to free stuff that was created by a driver, that will already be dealt with when the datafeed callback returns.

4. The straightforward solution for the moment is to add some flag to indicate whether the structures associated with a Packet/Payload are allocated by the bindings. Then in the destructor, free these if the flag is set.

5. We can't actually use sr_packet_free() for this, because it's not in the public API. Nor is sr_packet_copy(). I'm not sure why this is the case, since the clear use case for those functions is to allow a client to keep a copy of a packet that was passed to the datafeed callback, and then free it later. We don't use them internally at all. I've filed this as bug #1277.

6. Assuming sr_packet_{copy/free} are made public, there's an open question about how the bindings should expose that functionality. It would be nice if you could keep a copy of a packet simply by having its refcount not be zero once the datafeed callback returns. That would fit the usual expectations of a Python user, for instance.

7. We could easily make (6) work by catching the case where the C++ datafeed callback has returned to the C wrapper, but the refcount of the Packet/Payload is not zero. In that case, call sr_packet_copy() to copy the structures, update the C++ object to point to them, and change the flag to indicate this is now a user-owned object.

8. The problem with the approach in (7) is that any pointers to the raw data associated with the packet would now be wrong. I think the only way to deal with that nicely would be to expose something smarter than a bare void * for the payload data, which would mean an API change.