Callbacks duplicate registration + fix (in regards to soft restarts using sighup with different persistent config)

[EZenderink]

Edit to my earlier post with some more analysis and potential fix.

Goal: Our goal is to soft restart netsnmp while being able to switch persistent configurations each soft restart (sighup).
Problem: the following issues were noted when providing different name/type (to load a different persistent configuration):

• The following warnings on stderr:

  • Unknown token: ifXTable.
  • Unknown token: usmUser (usm)
  • Unknown token: createUser (usm)
  • Unknown token: rwuser (vacm, others as well)
  • Unknown token: serialno
  • duplicate registration: MIB modules ipAddressTable and ipAddressTable
  • (after fixing the above) duplicate registration: MIB modules ifXtable and ifXtable
  • (after fixing the above) duplicate registration: MIB modules snmpSetSerialNo and snmpSetSerialNo

• Growing persistent configuration without changes during soft restarts (SIGHUP)

• Memory leaks (after fixing the above) (asan):

    =================================================================
    ==32315==ERROR: LeakSanitizer: detected memory leaks
    
    Direct leak of 16 byte(s) in 2 object(s) allocated from:
        #0 0x7f326c0ca330 in __interceptor_malloc (/lib/x86_64-linux-gnu/libasan.so.5+0xe9330)
        #1 0x558e7f47738a in netsnmp_memdup /home/s4200ng-ipc/benchmark-in-house/Code/packages/net-    snmp/core/snmplib/tools.c:285
    
    SUMMARY: AddressSanitizer: 16 byte(s) leaked in 2 allocation(s).
  

Our current version:

SNMP Versions Supported:    1 2c 3
Building for:               linux
Net-SNMP Version:           5.9
Network transport support:  Callback Unix Alias TCP UDP TCPIPv6 UDPIPv6 IPv4Base SocketBase TCPBase UDPIPv4Base UDPBase IPBase IPv6Base
SNMPv3 Security Modules:     usm
Agent MIB code:            default_modules if-mib snmp-usm-dh-objects-mib =>  snmpv3mibs mibII notification target agent_mibs if-mib/ifTable if-mib/ifXTable snmp-usm-dh-objects-mib/usmDHUserKeyTable snmp-usm-dh-objects-mib/usmDHParameters
MYSQL Trap Logging:         unavailable
Embedded Perl support:      disabled
SNMP Perl modules:          disabled
SNMP Python modules:        disabled
Crypto support from:        crypto
Authentication support:     MD5 SHA1 SHA224 SHA256 SHA384 SHA512
Encryption support:         DES AES AES128 AES192 AES192C AES256 AES256C
Local DNSSEC validation:    disabled

For us to restart netsnmp using sighup while providing a different persistent configuration we currently have to do the following:

int RestartSnmp(void)
{
    // Shut down current instance
    shutdown_master_agent();

    // Call update config callbacks.
    snmp_call_callbacks(SNMP_CALLBACK_APPLICATION, SNMPD_CALLBACK_PRE_UPDATE_CONFIG, NULL);

    // snmp_call_callbacks(SNMP_CALLBACK_LIBRARY, SNMP_CALLBACK_STORE_DATA, NULL);
    snmp_store(SnmpAgent_confPersistent(NULL));

    // Free stored configuration within netsnmp lib.
    free_config();

    // Shutdown USM instance.
    shutdown_usm();

    // Unregister the token for this specific configuration
    // @note, this is to prevent tokens being handled for the wrong application
    // instance
    unregister_config_handler(SnmpAgent_confPersistent(NULL), "createUser");
    unregister_config_handler(SnmpAgent_confPersistent(NULL), "usmUser");

    // fix Warning: Unknown token: ifXTable.
    _ifXTable_shutdown_interface(ifXTable_registration_get());
    // shutdown_ifXTable();

    // restart logging
    netsnmp_logging_restart();

    // Make sure that the correct configs are read when restarting SNMP
    SnmpAgent_activateConf(aConfPersistent, SnmpAgent_confPersistent(NULL));

    // Set new configuration file.
    SnmpAgent_confPersistent(aConfPersistent);

    // unregister ipAddressTable
    // Prevent stderr warning:
    // duplicate registration: MIB modules ipAddressTable and ipAddressTable
    // (oid .1.3.6.1.2.1.4.34).
    unregister_mib((oid[]){1, 3, 6, 1, 2, 1, 4, 34}, 8);

    // Unregister ifXtable
    // duplicate registration: MIB modules ifXtable and ifXtable
    unregister_mib((oid[]){1, 3, 6, 1, 2, 1, 31, 1, 1}, 9);

    // Unregister snmpSetSerialNo (oid .1.3.6.1.6.3.1.1.6.1)
    // duplicate registration: MIB modules snmpSetSerialNo  and snmpSetSerialNo 
    unregister_mib((oid[]){1, 3, 6, 1, 6, 3, 1, 1, 6, 1}, 10);

    // Read new persistent config 
    init_agent_read_config(SnmpAgent_confPersistent(NULL));

    // Initialize configuration tokens for usm and vacm
    // fix createUser token unknown warning
    // @note: originally part of init_usm_conf(char* type), part of snmpusm.c,
    //        however this function  also registers the SNMPD_CALLBACK_STORE_DATE
    //        which causes duplicate USM entries in the configuration files!
    register_config_handler(
    SnmpAgent_confPersistent(NULL), "usmUser", usm_parse_config_usmUser, NULL, NULL);
    register_config_handler(SnmpAgent_confPersistent(NULL), "createUser", usm_parse_create_usmUser,
    NULL,
    "username [-e ENGINEID] (MD5|SHA|SHA-512|SHA-384|SHA-256|SHA-224|default) authpassphrase "
    "[(DES|AES|default) [privpassphrase]]");

    // fix Warning: Unknown token: rwuser (etc)
    init_vacm_conf();

    // fix Warning: Unknown token: ifXTable.
    _ifXTable_initialize_interface(ifXTable_registration_get(), 0);

    // fix Warning: Unknown token: setserialno.
    init_setSerialNo();

    // Read configurations (aka baseconfig)
    read_configs();

    // Start master again
    if (init_master_agent())
    {
        LOGS42_ERR("SNMPAGENT", "Failed to restart master agent!");
        return 1;
    }

    return 0;
}

This fixes all stderr warnings but not the growing persistent configuration files (duplicate entries for usm, ifXtable and serialno). This had to be fixed with change within how callbacks are registred in callback.c:

Some digging into the code showed that within callback.c during registration of callbacks it is not verified if the callback to be registered, is already present or not.

Adding the following code to not register duplicate callbacks fixes this issue (see comment in code):

/**
 * Register a callback function.
 *
 * @param major        Major callback event type.
 * @param minor        Minor callback event type.
 * @param new_callback Callback function being registered.
 * @param arg          Argument that will be passed to the callback function.
 * @param priority     Handler invocation priority. When multiple handlers have
 *   been registered for the same (major, minor) callback event type, handlers
 *   with the numerically lowest priority will be invoked first. Handlers with
 *   identical priority are invoked in the order they have been registered.
 *
 * @see snmp_register_callback
 */
int
netsnmp_register_callback(int major, int minor, SNMPCallback * new_callback,
                        void *arg, int priority)
{
    struct snmp_gen_callback *newscp = NULL, *scp = NULL;
    struct snmp_gen_callback **prevNext = &(thecallbacks[major][minor]);

    if (major >= MAX_CALLBACK_IDS || minor >= MAX_CALLBACK_SUBIDS) {
        return SNMPERR_GENERR;
    }

    if (_callback_need_init)
        init_callbacks();

    _callback_lock(major,minor, "netsnmp_register_callback", 1);

       //--------------------------------------------------------------------------
// Added check to prevent duplicate registrations of callbacks.
// Return SUCCESS if already registered, and free provided "arg"
// to prevent memory leaks.
for (scp = thecallbacks[major][minor]; scp != NULL; scp = scp->next)
{
    if (scp->sc_callback == new_callback && scp->priority == priority)
    {

        if (arg != NULL)
        {
            if(scp->sc_client_arg != NULL && scp->sc_client_arg != arg)
            {
                SNMP_FREE(scp->sc_client_arg);
            }
            scp->sc_client_arg = arg;
        }
        DEBUGMSGTL(("callback_duplicate",
         "Callback already registered (%d,%d) at %p with priority %d\n",
          major, minor, scp, priority));
        _callback_unlock(major, minor);
        return SNMPERR_SUCCESS;
    }
}
//--------------------------------------------------------------------------

    if ((newscp = SNMP_MALLOC_STRUCT(snmp_gen_callback)) == NULL) {
        _callback_unlock(major,minor);
        return SNMPERR_GENERR;
    } else {
        newscp->priority = priority;
        newscp->sc_client_arg = arg;
        newscp->sc_callback = new_callback;
        newscp->next = NULL;

        for (scp = thecallbacks[major][minor]; scp != NULL;
            scp = scp->next) {
            if (newscp->priority < scp->priority) {
                newscp->next = scp;
                break;
            }
            prevNext = &(scp->next);
        }

        *prevNext = newscp;

        DEBUGMSGTL(("callback", "registered (%d,%d) at %p with priority %d\n",
                    major, minor, newscp, priority));
        _callback_unlock(major,minor);
        return SNMPERR_SUCCESS;
    }
}

This prevents multiple registrations of the same callback (in general) and frees arguments if they should be replaced.

Having to manually unregister, and reinitialize mibs such as ifXtable and serialno does not seem the "correct" way of doing this, but I haven't found another solution to make our goal work. If there are suggestions or solutions available that would help achieve our goal that is more "official", I would be happy to know.

If required I can create a pull request, but in case that I missed some edge scenario where the way this is done is not recommended I'd like this to be discussed first. Currently we do not see functional issues with netsnmp while using this fix (Netsnmp test suite passes with no errors).