Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libindiclient leaks last blob on driver restart #1528

Closed
pludov opened this issue Aug 29, 2021 · 10 comments
Closed

libindiclient leaks last blob on driver restart #1528

pludov opened this issue Aug 29, 2021 · 10 comments
Assignees

Comments

@pludov
Copy link
Contributor

pludov commented Aug 29, 2021

Upon removal of devices or property, blob data are not freed. As a result, a memory leak of blob will occurs when a driver that has received blob is restarted:

  1. Compile the dummy-streamer example program joined
  2. Start an indi server with ccd_simulator
  3. Start dummy-streamer
  4. Activate streaming on the ccd_simulator: dummy-streamer logs it blob receptions
  5. Restart the ccd_simulator (through indi fifo)
  6. Restart streaming on the ccd_simulator

Repeating 5&6 will make dummy-streamer program memory size. You can use bigger frame size to make the problem more obvious. Alternatively, running dummy-streamer with valgrind will report the following leak:

==26462== 1,328,925 bytes in 1 blocks are definitely lost in loss record 28 of 28
==26462==    at 0x4C33D2F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==26462==    by 0x11FCC3: INDI::BaseDevice::setBLOB(_IBLOBVectorProperty*, xml_ele_*, char*) (basedevice.cpp:731)
==26462==    by 0x122837: INDI::BaseDevice::setValue(xml_ele_*, char*) (basedevice.cpp:692)
==26462==    by 0x10FF1D: INDI::BaseClientPrivate::dispatchCommand(xml_ele_*, char*) (baseclient.cpp:579)
==26462==    by 0x110CF5: INDI::BaseClientPrivate::listenINDI() (baseclient.cpp:440)
==26462==    by 0x53376DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==26462==    by 0x4E456DA: start_thread (pthread_create.c:463)
==26462==    by 0x5CDA71E: clone (clone.S:95)

My though: blob data freeing is commented out in indipropertyview.h, so there is simply no free on removal. I am not sure the impact of activating it back however.

template <>
struct WidgetView<IBLOB>: PROPERTYVIEW_BASE_ACCESS IBLOB
{
    ...
    ~WidgetView()                                          { /* free(this->blob); */ }
    void clear()                                           { /* free(this->blob); */ memset(this, 0, sizeof(*this)); }
@knro
Copy link
Contributor

knro commented Aug 29, 2021

It should be clear. Not sure what @pawel-soja has intended exactly with this? Can you uncomment and test it out? Maybe because the BLOB could be shared with the client still so freeing the buffer would cause issues in the client at this stage... but we are responsible for freeing any memory we allocate.

@knro
Copy link
Contributor

knro commented Sep 28, 2021

Ok looks like @pawel-soja is busy. Did you try uncommenting the free and check for any possible regressions?

@knro
Copy link
Contributor

knro commented Nov 22, 2022

@pawel-soja was this issue fixed in your latest PR?

@pawel-soja
Copy link
Contributor

@knro sure i will check it out, but after i finish rework in CMakeLists.

@knro
Copy link
Contributor

knro commented Feb 27, 2023

@pawel-soja Any confirmation on whether this issue is resolved?

@pawel-soja
Copy link
Contributor

Hey, I analyzed the problem.

The WidgetView class cannot free memory because it is just an access frontend. When creating these classes, I left the behavior as it was before, i.e. whoever created the Blob managed the memory for the data. My goal was to create INDI::PropertyXXX that manages the allocation completely, but in the case of Blob there were some difficulties, each piece of code allocates and frees memory differently, additionally there was an additional complexity in the form of:

#ifdef ENABLE_INDI_SHARED_MEMORY
IDSharedBlobFree(widget->getBlob());
#else
free(widget->getBlob());
#endif

indi/libs/indibase/stream/encoder/rawencoder.cpp
image

So, as I mentioned, I left the Property behavior unchanged so as not to free someone's memory.

There are 2 choices, either fix the BaseClient implementation so that it frees memory when the Property is destroyed, or, more future-proof, create a memory management implementation on the Property side. Unfortunately, with the second option, I can't guarantee that something won't crash with foreign programs that allocate memory and give a pointer to PropertyBlob.

@pawel-soja
Copy link
Contributor

There is also an option to add the setBlobDeleter function so that the programmer decides how the memory should be freed.

// A
{
    INDI::PropertyBlob blob{1};
    blob.setBlobDeleter([](void *data)
    {
        free(data); // correct for malloc
    });

    blob[0].setBlob(malloc(128));
}

// B
{
    INDI::PropertyBlob blob{1};
    blob.setBlobDeleter([](void *data)
    {
        delete[] reinterpret_cast<char*>(data); // correct for new operator[]
    });

    blob[0].setBlob(new char[128]);
}

@knro
Copy link
Contributor

knro commented Mar 1, 2023

I like this deleter option as it gives more freedom on how should be cleared on property destruction.

Copy link

This issue has been inactive for 60 days and is being marked as stale.

@github-actions github-actions bot added the Stale label Apr 11, 2024
Copy link

This issue has been closed due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants