#2652 net-snmp (5.7.3) uses strtok in a thread-unsafe manner, leading to crashes

[Timothy Miller]

I have been having trouble with the use of libnetsnmp and crashes, and I have tracked this down. Here is what valgrind reports:

==6318== Invalid read of size 1
==6318== at 0x71073F: strtok (in /lib/libc-2.12.so)
==6318== by 0x8D1B31: snmp_sess_open (snmp_api.c:1563)
==6318== by 0x8073335: SnmpTools::OpenSession(char, long) (snmptools.cpp:149)
==6318== by 0x8073DD3: snmpget(char, char, char, long) (snmptools.cpp:515)
==6318== by 0x8055992: proxyStr::get_request(Agentpp::Request, int) (snmpProxy.cpp:251)
==6318== by 0x407E47E: Agentpp::Mib::process_request(Agentpp::Request, int) (mib.cpp:3227)
==6318== by 0x407A1CB: Agentpp::Mib::do_process_request(Agentpp::Request) (mib.cpp:3484)
==6318== by 0x40B6445: Agentpp::MibTask::run() (threads.cpp:1039)
==6318== by 0x40B6D4C: Agentpp::TaskManager::run() (threads.cpp:830)
==6318== by 0x40B8127: Agentpp::thread_starter(void) (threads.cpp:539)
==6318== by 0x83CB38: start_thread (in /lib/libpthread-2.12.so)
==6318== by 0x778D6D: clone (in /lib/libc-2.12.so)
==6318== Address 0x42cf800 is 8 bytes inside a block of size 9 free'd
==6318== at 0x4006CAF: free (vg_replace_malloc.c:446)
==6318== by 0x8EB97F: netsnmp_tdomain_transport_full (snmp_transport.c:655)
==6318== by 0x8D1B31: snmp_sess_open (snmp_api.c:1563)
==6318== by 0x8073335: SnmpTools::OpenSession(char, long) (snmptools.cpp:149)
==6318== by 0x8073EC3: snmpget(char, char, int, long) (snmptools.cpp:525)
==6318== by 0x8055D1E: proxyInt::get_request(Agentpp::Request, int) (snmpProxy.cpp:183)
==6318== by 0x407E47E: Agentpp::Mib::process_request(Agentpp::Request, int) (mib.cpp:3227)
==6318== by 0x407A1CB: Agentpp::Mib::do_process_request(Agentpp::Request) (mib.cpp:3484)
==6318== by 0x40B6445: Agentpp::MibTask::run() (threads.cpp:1039)
==6318== by 0x40B6D4C: Agentpp::TaskManager::run() (threads.cpp:830)
==6318== by 0x40B8127: Agentpp::thread_starter(void) (threads.cpp:539)
==6318== by 0x83CB38: start_thread (in /lib/libpthread-2.12.so)

The bug is in netsnmp_tdomain_transport_full (snmp_transport.c), starting at line 598, we have this code:

            while (*++cp) if (*cp == ',') commas++;
            lspec = calloc(commas+2, sizeof(char *));
            commas = 1;
            lspec[0] = strtok(dup, ",");
            while ((lspec[commas++] = strtok(NULL, ",")))
                ;
            spec = (const char * const *)lspec;

Notice that there is a call to strtok with a NULL pointer. In this case, strtok uses a static buffer. This is not thread-safe creating a race condition if more than one thread is using this function at once. For my program, this happens perhaps once every six hours when I'm stress-testing.

This looks like it'll be really easy to fix, so I'll post a fix in a follow-up. Basically, we should just avoid strtok and implement the functionality directly in C code.

[Timothy Miller]

strtok_r is the correct function to use, so this should work correctly:

            while (*++cp) if (*cp == ',') commas++;
            lspec = calloc(commas+2, sizeof(char *));
            commas = 1;
            char *saveptr;
            lspec[0] = strtok_r(dup, ",", &saveptr);
            while ((lspec[commas++] = strtok_r(NULL, ",", &saveptr)))
                ;
            spec = (const char * const *)lspec;

[Niels Baggesen]

Thanks for the note. This has recently been been fixed in response to bug 2217.