sdrangel icon indicating copy to clipboard operation
sdrangel copied to clipboard

Fix memory leaks

Open dforsi opened this issue 1 year ago • 5 comments

This PR fixes some memory leaks found with AddressSanitizer (https://github.com/google/sanitizers/wiki/AddressSanitizer).

Most fixes consist only of adding a missing delete.

dforsi avatar Apr 07 '24 07:04 dforsi

I removed my changes the the Radiosonde plugin. Should I remove other changes? @srcejon did you push your changes to a public repository? Maybe I can apply your changes to other plugins?

dforsi avatar Apr 29 '24 17:04 dforsi

Yes, the Radiosonde delegate changes have been committed.

I think the other changes are fine as far as I can see.

RE the loop in Airline::Init::~Init(), I think you should just be able to do qDeleteAll(m_icaoHash); - Not important, just FYI!

srcejon avatar Apr 29 '24 17:04 srcejon

I added some qDeleteAll() that free the memory used by the container but not the object referenced, for example qDeleteAll(*m_airportInfo); clears the hash but it doesn't delete the single AirportInformation objects (as expected), so what should be done in this case? Since the objects are already in a container maybe it's better to iterate over the hash and delete them one by one instead of using parent that would just duplicate the references? And m_substitutions in Webserver is similar even if there are only 2 items. On the other hand, the objects used to create the charts in the Sky Map plugin seem to be the case for parent?

One of the commits of this PR fixes a memleak in the handling of a Message in afcgui.cpp where a return true; was missing but why almost all the code in handleMessage() checks for

    while ((message = getInputMessageQueue()->pop()))
    {
        if (handleMessage(*message)) {
            delete message;
        }
    }

If the message is not handled there is a bug somewhere and I would rather issue a warning and delete the message anyway to avoid a memory leak.

the only places where the message is always deleted seem to be:

$ grep -rn 'handleMessage(' --include=*.{cpp,h} | grep -v bool | grep -v if
sdrsrv/mainserver.cpp:247:        handleMessage(*message);
sdrgui/mainwindow.cpp:2055:             handleMessage(*message);
plugins/channelrx/demoddsd/dsddemodsink.cpp:240:                        m_ambeFeature->handleMessage(*msg);
plugins/channelrx/demoddsd/dsddemodsink.cpp:260:                        m_ambeFeature->handleMessage(*msg

dforsi avatar May 04 '24 17:05 dforsi

I added some qDeleteAll() that free the memory used by the container but not the object referenced, for example qDeleteAll(*m_airportInfo); clears the hash but it doesn't delete the single AirportInformation objects (as expected), so what should be done in this case?

You shouldn't try to delete m_airportInfo - that uses a QSharedPointer so can be shared - and it should have automatic deletion: See: https://doc.qt.io/qt-6/qsharedpointer.html

srcejon avatar May 07 '24 13:05 srcejon

If the message is not handled there is a bug somewhere and I would rather issue a warning and delete the message anyway to avoid a memory leak.

Does a return value of false indicate that the message wasn't handled, or that delete shouldn't be called?

E.g. the handler could forward the message on to another queue, rather than allocate a new message. In that case, the message was handled, but you wouldn't want it deleted.

srcejon avatar May 07 '24 13:05 srcejon

If the message is not handled there is a bug somewhere and I would rather issue a warning and delete the message anyway to avoid a memory leak.

Does a return value of false indicate that the message wasn't handled, or that delete shouldn't be called?

E.g. the handler could forward the message on to another queue, rather than allocate a new message. In that case, the message was handled, but you wouldn't want it deleted.

it returns false to indicate that the message wasn't handled, but its caller that decides to delete or not is void AFCGUI::handleInputMessages() and it doesn't call other methods, so the memory is leaked.

dforsi avatar May 18 '24 17:05 dforsi

I dropped the changes to the Map plugin because I still need to see how to free the single map items and this PR has been opened for too long.

I will look into Map and SID in separate PRs and maybe others unless someone beats me to it :).

dforsi avatar May 19 '24 17:05 dforsi

Looks OK, thanks.

srcejon avatar May 19 '24 21:05 srcejon