[AGENT++] [SNMP++] Various bug fixes & observations

[Stephen Agate]

These comments relate to SNMP++ V3.2.25 and AGENT++ V3.5

#1  [BUFFER OVERRUN]

The write at "..\snmp++\src\auth_priv.cpp" at line 2185 is past end of the 
array.

    unsigned int enc_buffer_len = 80;
    unsigned int dec_buffer_len = 80;
    ...
    dec_buffer[dec_buffer_len] = 0;



#2  [ENHANCEMENT]

In "..\snmp++\src\uxsnmp.cpp" change the start of Snmp::process_thread() 
from:

    #ifdef WIN32
    int Snmp::process_thread(Snmp *pSnmp)
    {
    #else
    void* Snmp::process_thread(void *arg)
    {
        Snmp* pSnmp = (Snmp*) arg;
    #endif // !WIN32

        // Loop as long as we haven't stopped
        while (pSnmp->is_running())
        {

to:

    #ifdef WIN32
    int Snmp::process_thread(Snmp *pSnmp)
    {
    #ifdef _THREADS
        ::ResetEvent(pSnmp->m_hThreadEndEvent);
    #endif
    #else
    void* Snmp::process_thread(void *arg)
    {
        Snmp* pSnmp = (Snmp*) arg;
    #endif // !WIN32

        // Loop as long as we haven't stopped
        while (pSnmp->is_running())
        {

This permits the thread to be repeatedly started and stopped with 
Snmp::start_poll_thread() and Snmp::stop_poll_thread(). Without the patch, 
the call to ::WaitForSingleObject() within Snmp::stop_poll_thread() will be 
immediately satisfied after the first pass, which could permit the Snmp 
object to be deleted at a higher level whilst the thread is still running.



#3  [Memory leak]

In "...\agent++\include\agent_pp\request.h", at line 639, change 
delete_lock_queue() to a static member. This can then be used without an 
object to tidy up memory before the program terminates:

    // Loop until requested to exit
    while (!TestDestroy())
    {
        Request* pcRequest=pcReqList->receive(1);
        if (pcRequest)
        {
            pcMIB->process_request(pcRequest);
        }
        else
        {
            pcMIB->cleanup();
        }
    }

    // Tidy up
    Request::delete_lock_queue();   // Needed to avoid memory leak
    delete pcReqList;
    delete pcMIB;



#4  [HEADER FIX]

Change "...\snmp++\include\snmp_pp\address.h" line 86 from:

    #if !defined(_AIX) && !defined(__QNX_NEUTRINO)

to:

    #if !defined(_AIX)

This modification removes the QNX specialisation around the inclusion of the 
"unistd.h" header. From QNX Version 6.3.0* (possibly earlier) the close() 
function is correctly defined within the header (i.e. they are now POSIX 
compliant).
(*) Released in 2004.



#5  [UNUSED & UNREACHABLE CODE]

In "..\agent++\src\mib.cpp", the constructor "MibTable::MibTable(const Oidx& 
o, unsigned int ilen, boolean a)" does not use the boolean parameter. Also, 
the implementation contains unreachable code, since the the ilen parameter 
is unsigned. Similar unreachable code is also present within the constructor 
"MibTable::MibTable(const Oidx& o, unsigned int ilen)".



#6  [OBSERVATION]

Only one MibTable constructor allows the user to correctly specify a 
multi-column table index, in order for MibTable::is_index_valid() to work 
with a multi-column table index.

To correctly specify a multi-column table index, the 
MibTable::MibTable(const Oidx& o, const index_info* istruc, unsigned int 
ilen) constructor must be used.

It is the index_info member variable of MibTable that specifies the index 
for the corresponding MIB table. The other constructors always create an 
index_info array of size 1, indicating an index consisting of a single item. 
This mostly works because the type of the index_info’s single entry is 
always simply defined as an OID.

However, error checking such an index (via a call to the 
MibTable::is_index_valid() function) is limited, since nothing is known 
about the items that make up the OID. For example:

Consider a table with an index that is made up of an IP address and a port 
number. One possible index would be 1.2.3.4.4000 (IP address of 1.2.3.4 and 
a port number of 4000). If treated as an OID, only the length of the index 
can be validated (i.e. it must always be of length 5). Thus, an index of 
256.2.3.4.4000 would also be considered valid, even though the IP address is 
actually invalid.

The constructor above allows the user to create and populate an index_info 
structure first and pass this into the call to the constructor. So, for the 
above example, an index_info array of size 2 can be created. The first entry 
in the array can be set to sNMP_SYNTAX_IPADDR. The second entry can be set 
to sNMP_SYNTAX_UINT32.



Regards,
Stephen