Tackling memory leaks #139

kamakazikamikaze · 2022-03-15T04:35:15Z

kamakazikamikaze
Mar 15, 2022
Maintainer

I've created branch memork-leaks to address outstanding issues regarding memory leaks. The first commit appears to reduce leaks from 13,684 bytes in 261 blocks to 10,044 bytes in 248 blocks. It's not much but it's a start. The next chunk of leaks I need to tackle seem to originate from v3 sessions:

62 bytes in 62 blocks are definitely lost in loss record 600 of 786
   at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x8774B25: hex_to_binary2 (tools.c:470)
   by 0x86E2ED2: netsnmp_create_session_v3 (interface.c:1693)
   by 0x3E2546: cfunction_call (methodobject.c:552)
   by 0x179297: _PyObject_MakeTpCall (call.c:215)
   by 0x26935E: _PyObject_VectorcallTstate (abstract.h:112)
   by 0x2693D9: PyObject_Vectorcall (abstract.h:123)
   by 0x2824B5: trace_call_function (ceval.c:5822)
   by 0x28280A: call_function (ceval.c:5864)
   by 0x27C31A: _PyEval_EvalFrameDefault (ceval.c:4181)
   by 0x269679: _PyEval_EvalFrame (pycore_ceval.h:46)
   by 0x280714: _PyEval_Vector (ceval.c:5065)

62 bytes in 62 blocks are definitely lost in loss record 601 of 786
   at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x8774B25: hex_to_binary2 (tools.c:470)
   by 0x86E2F20: netsnmp_create_session_v3 (interface.c:1696)
   by 0x3E2546: cfunction_call (methodobject.c:552)
   by 0x179297: _PyObject_MakeTpCall (call.c:215)
   by 0x26935E: _PyObject_VectorcallTstate (abstract.h:112)
   by 0x2693D9: PyObject_Vectorcall (abstract.h:123)
   by 0x2824B5: trace_call_function (ceval.c:5822)
   by 0x28280A: call_function (ceval.c:5864)
   by 0x27C31A: _PyEval_EvalFrameDefault (ceval.c:4181)
   by 0x269679: _PyEval_EvalFrame (pycore_ceval.h:46)
   by 0x280714: _PyEval_Vector (ceval.c:5065)

4,960 bytes in 62 blocks are definitely lost in loss record 744 of 786
   at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x8759934: snmp_duplicate_objid (snmp_api.c:7909)
   by 0x86E301A: netsnmp_create_session_v3 (interface.c:1722)
   by 0x3E2546: cfunction_call (methodobject.c:552)
   by 0x179297: _PyObject_MakeTpCall (call.c:215)
   by 0x26935E: _PyObject_VectorcallTstate (abstract.h:112)
   by 0x2693D9: PyObject_Vectorcall (abstract.h:123)
   by 0x2824B5: trace_call_function (ceval.c:5822)
   by 0x28280A: call_function (ceval.c:5864)
   by 0x27C31A: _PyEval_EvalFrameDefault (ceval.c:4181)
   by 0x269679: _PyEval_EvalFrame (pycore_ceval.h:46)
   by 0x280714: _PyEval_Vector (ceval.c:5065)

4,960 bytes in 62 blocks are definitely lost in loss record 745 of 786
   at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x8759934: snmp_duplicate_objid (snmp_api.c:7909)
   by 0x86E31ED: netsnmp_create_session_v3 (interface.c:1769)
   by 0x3E2546: cfunction_call (methodobject.c:552)
   by 0x179297: _PyObject_MakeTpCall (call.c:215)
   by 0x26935E: _PyObject_VectorcallTstate (abstract.h:112)
   by 0x2693D9: PyObject_Vectorcall (abstract.h:123)
   by 0x2824B5: trace_call_function (ceval.c:5822)
   by 0x28280A: call_function (ceval.c:5864)
   by 0x27C31A: _PyEval_EvalFrameDefault (ceval.c:4181)
   by 0x269679: _PyEval_EvalFrame (pycore_ceval.h:46)
   by 0x280714: _PyEval_Vector (ceval.c:5065)

There are a lot of "possibly lost" blocks that do not seem to tie directly back to any invoking code from interface.c, so for now I'm going to assume that these are caused from strings created in Python that get passed to our v3 sessions and are lost because they are not able to be safely deallocated. So hopefully tackling the "definitely lost" results in clearing the "possibly lost".

carlkidcrypto · 2022-03-15T15:35:08Z

carlkidcrypto
Mar 15, 2022

Glad to hear work is being done to address the memory leaks. For reference to myself and others. Here is the memory leak I see on 0.2.5 on Ubuntu 20. I have a python script that runs snmp queries repeatedly once every second on 16 snmp enabled devices. I had to write a systemd service file that would restart my logger once every 4 hours. Otherwise the memory profile grows significantly. I'll swap over to the memory leak branch and see what the difference is.

Update: Switched to the memory leak branch. Now we wait and see 👍🏽

24 replies

kamakazikamikaze Mar 19, 2022
Maintainer Author

Alright, pushed a0ef2e to memory-leaks. Check it out and see if it helps!

kamakazikamikaze Mar 21, 2022
Maintainer Author

Also, @carlkid1499 you could probably reduce memory consumption by holding on to the sessions between runs. Each session appears to re-initialize the SNMP library which doesn't necessarily leak memory but may contribute to the residual size. If you'd like assistance in refactoring to make this work I'd be more than happy to help provide some examples.

carlkidcrypto Mar 21, 2022

Also, @carlkid1499 you could probably reduce memory consumption by holding on to the sessions between runs. Each session appears to re-initialize the SNMP library which doesn't necessarily leak memory but may contribute to the residual size. If you'd like assistance in refactoring to make this work I'd be more than happy to help provide some examples.

Thanks for doing all that work. Just now pulling the changes down for some tests. Yeah, I could get by with not deleting the session. It's being deleted since 16 threads are launched to collect snmp stats from all 16 devices simultaneously. To be safe and thread safe I make and destroy each session within the threads. So really the logic is like:

pass parameters to thread func (not the easysnmp session) i.e IP of device.
each thread executes making one session and then fetching several distinct OIDs
each thread ends with deleting its session.
main thread joins all threads back
Sleep and then repeat.

kamakazikamikaze Mar 21, 2022
Maintainer Author

Thanks for doing all that work. Just now pulling the changes down for some tests. Yeah, I could get by with not deleting the session. It's being deleted since 16 threads are launched to collect snmp stats from all 16 devices simultaneously. To be safe and thread safe I make and destroy each session within the threads. So really the logic is like:

pass parameters to thread func (not the easysnmp session) i.e IP of device.

each thread executes making one session and then fetching several distinct OIDs

each thread ends with deleting its session.

main thread joins all threads back

Sleep and then repeat.

Oof, yeah ok I understand now why you're doing that. Way back when I used this library for work, we had to spawn multiple processes and assign one target device per process, then set a limit of max tasks per process so memory could be free'd back to the system. (We were querying 3000+ devices per run every 15 minutes.) I don't think you'd want to move in that direction.

Maybe I can see about making a session manager that initializes the library and tracks sessions, that way you can have a thread-safe way to query multiple devices and not need to destroy sessions repeatedly. No guarantees tho :/

carlkidcrypto Mar 21, 2022

Thanks for doing all that work. Just now pulling the changes down for some tests. Yeah, I could get by with not deleting the session. It's being deleted since 16 threads are launched to collect snmp stats from all 16 devices simultaneously. To be safe and thread safe I make and destroy each session within the threads. So really the logic is like:

pass parameters to thread func (not the easysnmp session) i.e IP of device.

each thread executes making one session and then fetching several distinct OIDs

each thread ends with deleting its session.

main thread joins all threads back

Sleep and then repeat.

Oof, yeah ok I understand now why you're doing that. Way back when I used this library for work, we had to spawn multiple processes and assign one target device per process, then set a limit of max tasks per process so memory could be free'd back to the system. (We were querying 3000+ devices per run every 15 minutes.) I don't think you'd want to move in that direction.

Maybe I can see about making a session manager that initializes the library and tracks sessions, that way you can have a thread-safe way to query multiple devices and not need to destroy sessions repeatedly. No guarantees tho :/

Wow 3000+ devices that's a huge work load!. For sure, for now it works as is. Making a session manager would be awesome, but IMHO not needed as of right now. Plus debugging session managers...... well I'll leave it at that lol.

But seriously, thank you for all this work to find the memory leaks!

kamakazikamikaze · 2022-03-16T03:34:33Z

kamakazikamikaze
Mar 16, 2022
Maintainer Author

My valgrind output definitely appears to only have issues with v3 sessions. Commenting out SNMPv3 session arguments shows zero definitely lost blocks.

It's honestly strange as I comb through the code. Referencing the above blocks:

/* lines 1692 to 1697, for blocks 600 to 601 */
    session.securityEngineIDLen =
        hex_to_binary2((unsigned char *)sec_eng_id, STRLEN(sec_eng_id),
                       (char **) &session.securityEngineID);
    session.contextEngineIDLen =
        hex_to_binary2((unsigned char *)context_eng_id, STRLEN(sec_eng_id),
                       (char **) &session.contextEngineID);

/* lines 1719 to 1723, for block 744 */
    else if (!strcmp(auth_proto, "DEFAULT"))
    {
        const oid* a = get_default_authtype(&session.securityAuthProtoLen);
        session.securityAuthProto
            = snmp_duplicate_objid(a, session.securityAuthProtoLen);
    }

/* lines 1765 to 1770, for block 745 */
    else if (!strcmp(priv_proto, "DEFAULT"))
    {
        const oid *p = get_default_privtype(&session.securityPrivProtoLen);
        session.securityPrivProto =
            snmp_duplicate_objid(p, session.securityPrivProtoLen);
    }

session.securityEngineID and session.contextEngineID are created in Net-SNMP tools.c:470 with char *s = calloc(1, olen ? olen : 1), *op = s; and destroyed in Net-SNMP snmp_api.c:1928-1953:

static void
snmp_free_session(netsnmp_session * s)
{
    if (s) {
        SNMP_FREE(s->localname);
        SNMP_FREE(s->peername);
        SNMP_FREE(s->community);
        SNMP_FREE(s->contextEngineID);
        SNMP_FREE(s->contextName);
        SNMP_FREE(s->securityEngineID);
        SNMP_FREE(s->securityName);
        SNMP_FREE(s->securityAuthProto);
        SNMP_FREE(s->securityPrivProto);
        SNMP_FREE(s->paramName);
#ifndef NETSNMP_NO_TRAP_STATS
        SNMP_FREE(s->trap_stats);
#endif /* NETSNMP_NO_TRAP_STATS */

        /*
         * clear session from any callbacks
         */
        netsnmp_callback_clear_client_arg(s, 0, 0);

        free((char *) s);
    }
}

I'll have to read up on calloc/malloc behaviors this week and see if we're passing something to the functions that violate their intended use cases.

1 reply

kamakazikamikaze Mar 20, 2022
Maintainer Author

Using documentation from the Net-SNMP developer's guide, it appears that some calls for dynamic value management are unnecessary. Replacing them by pointing to constants instead appears to plug most leaks. I'm just down to fixing the hex_to_binary2 leaks.

kamakazikamikaze · 2022-03-21T02:22:39Z

kamakazikamikaze
Mar 21, 2022
Maintainer Author

I believe I've detected the source of the leaks for v3 sessions. It appears that calling snmp_sess_open eventually results in our session being cloned and the original left unfree'd, rather than having our session added to the list of internal sessions as it is. The calling order is as follows: snmp_sess_open -> _sess_open -> snmp_sess_add -> snmp_sess_add_ex -> snmp_sess_copy -> _sess_copy. The solution appears to be to free our malloc calls in our create_session_capsule function after we've opened the session.

0 replies

kamakazikamikaze · 2022-04-24T19:36:43Z

kamakazikamikaze
Apr 24, 2022
Maintainer Author

Outstanding issues revolving around memory leaks have been addressed. This conversation will be locked for archival purposes and may be unlocked in the future if the issue(s) resurface.

0 replies

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tackling memory leaks #139

{{title}}

Replies: 4 comments 25 replies

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

Select a reply

Tackling memory leaks #139

kamakazikamikaze Mar 15, 2022 Maintainer

Replies: 4 comments · 25 replies

carlkidcrypto Mar 15, 2022

kamakazikamikaze Mar 19, 2022 Maintainer Author

kamakazikamikaze Mar 21, 2022 Maintainer Author

carlkidcrypto Mar 21, 2022

kamakazikamikaze Mar 21, 2022 Maintainer Author

carlkidcrypto Mar 21, 2022

kamakazikamikaze Mar 16, 2022 Maintainer Author

kamakazikamikaze Mar 20, 2022 Maintainer Author

kamakazikamikaze Mar 21, 2022 Maintainer Author

kamakazikamikaze Apr 24, 2022 Maintainer Author

kamakazikamikaze
Mar 15, 2022
Maintainer

Replies: 4 comments 25 replies

carlkidcrypto
Mar 15, 2022

kamakazikamikaze Mar 19, 2022
Maintainer Author

kamakazikamikaze Mar 21, 2022
Maintainer Author

kamakazikamikaze Mar 21, 2022
Maintainer Author

kamakazikamikaze
Mar 16, 2022
Maintainer Author

kamakazikamikaze Mar 20, 2022
Maintainer Author

kamakazikamikaze
Mar 21, 2022
Maintainer Author

kamakazikamikaze
Apr 24, 2022
Maintainer Author