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

Bug: Memory leaks reported by mumble #81

Closed
hbeni opened this issue Feb 16, 2021 · 13 comments · Fixed by #83
Closed

Bug: Memory leaks reported by mumble #81

hbeni opened this issue Feb 16, 2021 · 13 comments · Fixed by #83
Assignees
Labels
bug Something isn't working low prio Low priority mumble-plugin Affecting mumble plugin
Milestone

Comments

@hbeni
Copy link
Owner

hbeni commented Feb 16, 2021

Describe the bug
While debugging #78 we again got some messages reporting memory leaks:

FGCom [2021-02-16 12:44:11.327]: Shutdown plugin
FGCom [2021-02-16 12:44:11.327]: [DBG] stopping threads
FGCom [2021-02-16 12:44:11.327]: [DBG] sending UDP shutdown request to port 16661
FGCom [2021-02-16 12:44:11.327]: [UDP-server] shutdown command recieved, server stopping now
FGCom [2021-02-16 12:44:11.327]: [DBG] waiting for threads to finish
FGCom [2021-02-16 12:44:11.327]: [DBG] [UDP-server] thread finished.
FGCom [2021-02-16 12:44:11.372]: [DBG] ---------DEBUG THREAD FINISHED---------
FGCom [2021-02-16 12:44:11.372]: [DBG] [GC] thread finished
FGCom [2021-02-16 12:44:11.427]: [DBG] mumble_shutdown() complete.
<W>2021-02-16 12:44:11.429 PulseAudio: Forcibly disconnected from PulseAudio
<W>2021-02-16 12:44:11.447 Clearing leaked memory from a plugin
<W>2021-02-16 12:44:11.447 Clearing leaked memory from a plugin
<W>2021-02-16 12:44:11.447 Clearing leaked memory from a plugin
<W>2021-02-16 12:44:11.447 Clearing leaked memory from a plugin
<W>2021-02-16 12:44:11.447 Clearing leaked memory from a plugin

This does not happen always.

  • It looks like a clean shutdown without server connection or udp-client connected works without leak.
  • Just connected to a server without joining "the channel" yields three messages (lcl and rmt state is emtpy, but two other clients were connected)
  • Joining the special channel yields four messages (lcl and rmt state is still emtpy)
  • Attaching a radioGUI yields five mesages, but only if mumble gets closed

This needs further research, where the memory is not freed.

@hbeni hbeni added bug Something isn't working mumble-plugin Affecting mumble plugin low prio Low priority labels Feb 16, 2021
@hbeni hbeni added this to the 1.0 milestone Feb 16, 2021
@Krzmbrzl
Copy link

I once again advise you to rework your plugin so it uses https://github.com/mumble-voip/mumble-plugin-cpp as a base. When using the cpp based API, memory management is automated and you never have to worry about it.

@hbeni
Copy link
Owner Author

hbeni commented Feb 16, 2021

Hi @Krzmbrzl thank you for the hint, i opened a ticket for that (#82) so it won't get overlooked.

However, i really want to learn and understand what causes this, because i want to better understand the pitfalls of C/C++ programming in order to get better :)
I think i do use the free function from the API for each ressource, and my other variables should be allocated on the stack und thus should be freed automatically when going out of scope. Also, i only do get the reported leak when the plugin did something (UDP-server etc), and i do neither new nor malloc, so i don't understand where it (potentially) comes from.

@hbeni
Copy link
Owner Author

hbeni commented Feb 16, 2021

A valgrind run: valgrind.txt

Lets me behind clueless.

@hbeni
Copy link
Owner Author

hbeni commented Feb 16, 2021

Another, more verbose file gives hints: valgrind-3.txt.gz
I will try to dig deeper into this.

Plugin built with: make CFLAGS+=-ggdb3 clean all-debug

valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose --log-file=/tmp/valgrind-3.txt ./mumble

@hbeni
Copy link
Owner Author

hbeni commented Feb 16, 2021

@Krzmbrzl i get pointed to my call of the sync status, but i don't know whats wrong with that:

==67279== 120 bytes in 1 blocks are still reachable in loss record 2,511 of 2,796
==67279==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
==67279==    by 0x65AC0E9: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.15.2)
==67279==    by 0x65AB0FD: QThread::currentThread() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.15.2)
==67279==    by 0x1D89F5: API::MumbleAPI::isConnectionSynchronized_v_1_0_x(unsigned int, int, bool*, std::promise<Mumble_ErrorCode>*) (API_v_1_0_x.cpp:178)
==67279==    by 0x1E1A83: API::isConnectionSynchronized_v_1_0_x(unsigned int, int, bool*) (API_v_1_0_x.cpp:1417)
==67279==    by 0x205C4747: fgcom_isConnectedToServer() (io_plugin.cpp:130)
==67279==    by 0x205C5B3A: notifyRemotes(int, FGCOM_NOTIFY_T, int, unsigned int) (io_plugin.cpp:147)
==67279==    by 0x205C82AD: fgcom_notifyThread() (io_plugin.cpp:570)
==67279==    by 0x205D5A88: __invoke_impl<void, void (*)()> (invoke.h:60)
==67279==    by 0x205D5A88: __invoke<void (*)()> (invoke.h:95)
==67279==    by 0x205D5A88: _M_invoke<0> (thread:264)
==67279==    by 0x205D5A88: operator() (thread:271)
==67279==    by 0x205D5A88: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)()> > >::_M_run() (thread:215)
==67279==    by 0x6AF8ECF: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==67279==    by 0x544BEA6: start_thread (pthread_create.c:477)
==67279==    by 0x6E54DEE: clone (clone.S:95)

code is:

bool fgcom_isConnectedToServer() {
    //pluginDbg("fgcom_isConnectedToServer(): checking connection");
    bool synchronized;
    int resCode = mumAPI.isConnectionSynchronized(ownPluginID, activeConnection, &synchronized);
    if (STATUS_OK != resCode) {
        return false;
    }
    return synchronized;
}

Should'nt the bool get freed automatically after the context to where it returned to is finished, because it's a stack variable?
Or do i miss something else here?

@Krzmbrzl
Copy link

Krzmbrzl commented Feb 16, 2021

You won't find this leak by using valgrind. This is not a leak caused by you calling new anywhere.

Instead these leaks are created if you e.g. ask for a user's name via API. The String for that name will be allocated on Mumble's side (via malloc), filled with content and then a pointer to that String (living on the Heap) is passed to you through that API function.
As Mumble can't know when you're done with that String, you have to call the freeMemory API function passing this pointer to it, so that Mumble can then call free on it.

The message you are seeing indicates that you did not call the free API function for a couple of such resources (Strings or arrays obtained via API) and thus the automatic fail-safe on Mumble's side kicks in (freeing the memory after all but only when Mumble is shutting down).

@hbeni
Copy link
Owner Author

hbeni commented Feb 16, 2021

So in this case for example, the bool is not freed after i returned it?

@Krzmbrzl
Copy link

No. Boolean values do not need freeing.
Only pointer types do and it does depend on the context. If you check each API function's documentation though, you'll find it mentioned there explicitly if something needs freeing.
As I said before: The things that always require freeing are returned Strings and arrays.

@hbeni
Copy link
Owner Author

hbeni commented Feb 17, 2021

Hm, however i looked trough all my invocations of mumAPI and couldn't find things not freed that are indicated from the header file comments...

Just an idea: You probably store the pointers in mumble somewhere, so you can later hook up the cleaning routine.
Would it be possible to store the name of the used API function besides the pointer, so you can print it to the terminal when cleaning up non-freed ones? That would tremendously help with debugging.

@hbeni
Copy link
Owner Author

hbeni commented Feb 17, 2021

@Krzmbrzl I could isolate a code part where it seems to trigger a leak:
Note, it does not print the message Failed to retrieve local user ID, so it's solely running in the normal path.
If I comment that, one message is gone...
What am i diung wrong here? (I think that kind of mistake is also the cause for the other messages I get.)

            mumble_userid_t localUser;
            if (mumAPI.getLocalUserID(ownPluginID, activeConnection, &localUser) != STATUS_OK) {
                pluginLog("Failed to retrieve local user ID");
                return EC_USER_NOT_FOUND; // abort online init - something horribly went wrong.
            } else {
                // update mumble session id to all known identities
                localMumId = localUser;
                fgcom_remotecfg_mtx.lock();
                for (const auto &idty : fgcom_local_client) {
                    fgcom_local_client[idty.first].mumid = localUser;
                }
                fgcom_remotecfg_mtx.unlock();
                pluginLog("got local clientID="+std::to_string(localUser));
                mumAPI.freeMemory(ownPluginID, &localUser);
            }

@Krzmbrzl
Copy link

That should be possible, yes.

@Krzmbrzl
Copy link

Krzmbrzl commented Feb 17, 2021

mumAPI.freeMemory(ownPluginID, &localUser);

What are you doing there? This doesn't seem to make any sense. Why are you trying to free the user ID? 👀
If you were to check for it, this should throw an error: https://github.com/Krzmbrzl/mumble/blob/deb74a6634898f68d37fd8ba58253acf08c7b43b/src/mumble/API_v_1_0_x.cpp#L150

As can be seen in the docs of that function you don't have to free the user's ID.

@hbeni
Copy link
Owner Author

hbeni commented Feb 17, 2021

😇

hbeni added a commit that referenced this issue Feb 17, 2021
hbeni added a commit that referenced this issue Feb 17, 2021
@hbeni hbeni mentioned this issue Feb 17, 2021
hbeni added a commit that referenced this issue Feb 17, 2021
hbeni added a commit that referenced this issue Feb 17, 2021
- removed obsolete mumAPI.free() calls
- added missing mumAPI.free() in fgcom_updateClientComment()
@hbeni hbeni self-assigned this Feb 17, 2021
@hbeni hbeni modified the milestones: Soon™ (=Future), 1.0 Feb 17, 2021
@hbeni hbeni closed this as completed in #83 Feb 17, 2021
hbeni added a commit that referenced this issue Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low prio Low priority mumble-plugin Affecting mumble plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants