From 6e1c20e87357f8447d5aadf620d177ad8bffc5da Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sun, 5 Jun 2022 15:30:55 +0200 Subject: [PATCH] SNMP Frontend, fix handling of snmpd down and memory leaks If snmpd is down, clixon_snmp does not start If snmpd stops, clixon_snmp quits Mem leaks fixed --- apps/snmp/snmp_handler.c | 2 + apps/snmp/snmp_lib.c | 126 +++++++++++++++++----- apps/snmp/snmp_lib.h | 6 +- apps/snmp/snmp_main.c | 220 ++++++++++++++++++++++++-------------- apps/snmp/snmp_register.c | 17 ++- test/lib.sh | 3 + test/test_snmp_get.sh | 3 +- 7 files changed, 269 insertions(+), 108 deletions(-) diff --git a/apps/snmp/snmp_handler.c b/apps/snmp/snmp_handler.c index 62fc41d1..4c8d5b57 100644 --- a/apps/snmp/snmp_handler.c +++ b/apps/snmp/snmp_handler.c @@ -340,6 +340,8 @@ snmp_scalar_set(clicon_handle h, ok: retval = 0; done: + if (api_path) + free(api_path); if (cb) cbuf_free(cb); if (xtop) diff --git a/apps/snmp/snmp_lib.c b/apps/snmp/snmp_lib.c index b4a00b6f..9ee62823 100644 --- a/apps/snmp/snmp_lib.c +++ b/apps/snmp/snmp_lib.c @@ -139,15 +139,51 @@ snmp_msg_int2str(int msg) return clicon_int2str(snmp_msg_map, msg); } -/*! Free clixon snmp handler struct +/*! Duplicate clixon snmp handler struct + * Use signature of libnetsnmp data_clone field of netsnmp_mib_handler in agent_handler.h + * @param[in] arg */ -int -snmp_handle_free(clixon_snmp_handle *sh) +void* +snmp_handle_clone(void *arg) { - if (sh->sh_cvk) - cvec_free(sh->sh_cvk); - free(sh); - return 0; + clixon_snmp_handle *sh0 = (clixon_snmp_handle *)arg; + clixon_snmp_handle *sh1 = NULL; + + if (sh0 == NULL) + return NULL; + if ((sh1 = malloc(sizeof(*sh1))) == NULL){ + clicon_err(OE_UNIX, errno, "malloc"); + return NULL; + } + memset(sh1, 0, sizeof(*sh1)); + if (sh0->sh_cvk && + (sh1->sh_cvk = cvec_dup(sh0->sh_cvk)) == NULL){ + clicon_err(OE_UNIX, errno, "cvec_dup"); + return NULL; + } + return (void*)sh1; +} + +/*! Free clixon snmp handler struct + * Use signature of libnetsnmp data_free field of netsnmp_mib_handler in agent_handler.h + * @param[in] arg + */ +void +snmp_handle_free(void *arg) +{ + clixon_snmp_handle *sh = (clixon_snmp_handle *)arg; + + if (sh != NULL){ + if (sh->sh_cvk) + cvec_free(sh->sh_cvk); + if (sh->sh_table_info){ + if (sh->sh_table_info->indexes){ + snmp_free_varbind(sh->sh_table_info->indexes); + } + free(sh->sh_table_info); + } + free(sh); + } } /*! Translate from YANG to SNMP asn1.1 type ids (not value) @@ -167,9 +203,9 @@ type_yang2asn1(yang_stmt *ys, int extended) { int retval = -1; - yang_stmt *yrestype; /* resolved type */ - char *restype; /* resolved type */ - char *origtype=NULL; /* original type */ + yang_stmt *yrestype; /* resolved type */ + char *restype; /* resolved type */ + char *origtype = NULL; /* original type */ int at; yang_stmt *ypath; yang_stmt *yref; @@ -186,6 +222,10 @@ type_yang2asn1(yang_stmt *ys, } if (yang_path_arg(ys, yang_argument_get(ypath), &yref) < 0) goto done; + if (origtype){ + free(origtype); + origtype = NULL; + } if (yang_type_get(yref, &origtype, &yrestype, NULL, NULL, NULL, NULL, NULL) < 0) goto done; restype = yrestype?yang_argument_get(yrestype):NULL; @@ -226,6 +266,8 @@ type_yang2asn1(yang_stmt *ys, *asn1_type = at; retval = 0; done: + if (origtype) + free(origtype); return retval; } @@ -251,10 +293,10 @@ type_snmp2xml(yang_stmt *ys, int retval = -1; char *cvstr; enum cv_type cvtype; - cg_var *cv; - yang_stmt *yrestype; /* resolved type */ - char *restype; /* resolved type */ - char *origtype=NULL; /* original type */ + cg_var *cv = NULL; + yang_stmt *yrestype; /* resolved type */ + char *restype; /* resolved type */ + char *origtype = NULL; /* original type */ clicon_debug(1, "%s", __FUNCTION__); if (valstr == NULL){ @@ -326,14 +368,17 @@ type_snmp2xml(yang_stmt *ys, goto fail; break; } - if ((*valstr = cv2str_dup(cv)) == NULL){ clicon_err(OE_UNIX, errno, "cv2str_dup"); goto done; } retval = 1; done: - clicon_debug(1, "%s %d", __FUNCTION__, retval); + clicon_debug(2, "%s %d", __FUNCTION__, retval); + if (cv) + cv_free(cv); + if (origtype) + free(origtype); return retval; fail: retval = 0; @@ -359,9 +404,9 @@ type_xml2snmp_pre(char *xmlstr0, { int retval = -1; - yang_stmt *yrestype; /* resolved type */ - char *restype; /* resolved type */ - char *origtype=NULL; /* original type */ + yang_stmt *yrestype; /* resolved type */ + char *restype; /* resolved type */ + char *origtype = NULL; /* original type */ char *str = NULL; int ret; @@ -402,6 +447,8 @@ type_xml2snmp_pre(char *xmlstr0, retval = 1; done: clicon_debug(2, "%s %d", __FUNCTION__, retval); + if (origtype) + free(origtype); return retval; fail: retval = 0; @@ -603,11 +650,11 @@ yang2xpath_cb(yang_stmt *ys, /*! Construct an xpath from yang statement * Recursively construct it to the top. - * @param[in] ys Yang statement - * @param[in] keyvec Array of [name,val]s as a cvec of key name and values - * @param[out] xpath Malloced xpath string, use free() after use - * @retval 0 OK - * @retval -1 Error + * @param[in] ys Yang statement + * @param[in] keyvec Array of [name,val]s as a cvec of key name and values + * @param[out] xpath Malloced xpath string, use free() after use + * @retval 0 OK + * @retval -1 Error * @note * 1. This should really be in a core .c file, like clixon_yang, BUT * 2. It is far from complete so maybe keep it here as a special case @@ -693,3 +740,34 @@ snmp_body2oid(cxobj *xi, cbuf_free(enc); return retval; } + +/*========== libnetsnmp-specific code =============== + * Peeks into internal lib global variables, may be sensitive to library change + */ +/*! Check if netsnmp is connected + * @retval 1 yes, running + * @retval 0 No, not running + * XXX: this peeks into the "main_session" global variable in agent/snmp_agent.c + * Tried to find API function but failed + */ +int +snmp_agent_check(void) +{ + extern netsnmp_session *main_session; + + return (main_session != NULL) ? 1 : 0; +} + +/*! Cleanup remaining libnetsnmb memory + * XXX: this peeks into the "tclist" global variable in snmplib/parse.c + * Tried to find API function but failed + */ +int +snmp_agent_cleanup(void) +{ + extern void *tclist; + + if (tclist) + free(tclist); + return 0; +} diff --git a/apps/snmp/snmp_lib.h b/apps/snmp/snmp_lib.h index d2c94d64..1177f8cf 100644 --- a/apps/snmp/snmp_lib.h +++ b/apps/snmp/snmp_lib.h @@ -52,6 +52,7 @@ struct clixon_snmp_handle { size_t sh_oidlen; char *sh_default; /* MIB default value leaf only */ cvec *sh_cvk; /* Index/Key variables */ + netsnmp_table_registration_info *sh_table_info; /* To mimic table-handler in libnetsnmp code*/ }; typedef struct clixon_snmp_handle clixon_snmp_handle; @@ -60,7 +61,8 @@ typedef struct clixon_snmp_handle clixon_snmp_handle; */ int snmp_access_str2int(char *modes_str); const char *snmp_msg_int2str(int msg); -int snmp_handle_free(clixon_snmp_handle *sh); +void *snmp_handle_clone(void *arg); +void snmp_handle_free(void *arg); int type_yang2asn1(yang_stmt *ys, int *asn1_type, int extended); int type_snmp2xml(yang_stmt *ys, netsnmp_variable_list *requestvb, @@ -71,6 +73,8 @@ int type_xml2snmp_pre(char *xmlstr, yang_stmt *ys, char **snmpstr); int type_xml2snmp(char *snmpstr, int *asn1type, u_char **snmpval, size_t *snmplen, char **reason); int yang2xpath(yang_stmt *ys, cvec *keyvec, char **xpath); int snmp_body2oid(cxobj *xi, cg_var *cv); +int snmp_agent_check(void); +int snmp_agent_cleanup(void); #endif /* _SNMP_LIB_H_ */ diff --git a/apps/snmp/snmp_main.c b/apps/snmp/snmp_main.c index 40b11af0..adc5d013 100644 --- a/apps/snmp/snmp_main.c +++ b/apps/snmp/snmp_main.c @@ -30,6 +30,13 @@ the terms of any one of the Apache License version 2 or the GPL. ***** END LICENSE BLOCK ***** + * This is the clixon_snmp daemon + * It assumes a netsnmp damon is running. + * - If netsnmp does not run, clixon_snmp will not start + * - If netsnmp dies, clixon_snmp will exit + * - If netsnmp is restarted, so should clixon_snmp be + * It is possible to be more resilient, such as setting a timer and trying again, in fact, libnetsnmp + * has some such mechanisms */ #ifdef HAVE_CONFIG_H @@ -56,11 +63,15 @@ /* clicon */ #include +#include "snmp_lib.h" #include "snmp_register.h" /* Command line options to be passed to getopt(3) */ #define SNMP_OPTS "hD:f:l:o:z" +/* Forward */ +static int clixon_snmp_input_cb(int s, void *arg); + /*! Return (hardcoded) pid file */ static char* @@ -84,6 +95,87 @@ clixon_snmp_sig_term(int arg) clixon_exit_set(1); } +/*! Clean and close all state of netconf process (but dont exit). + * Cannot use h after this + * @param[in] h Clixon handle + */ +static int +snmp_terminate(clicon_handle h) +{ + yang_stmt *yspec; + cvec *nsctx; + cxobj *x; + char *pidfile = clicon_snmp_pidfile(h); + + snmp_shutdown(__FUNCTION__); + shutdown_agent(); + snmp_agent_cleanup(); + clicon_rpc_close_session(h); + if ((yspec = clicon_dbspec_yang(h)) != NULL) + ys_free(yspec); + if ((yspec = clicon_config_yang(h)) != NULL) + ys_free(yspec); + if ((nsctx = clicon_nsctx_global_get(h)) != NULL) + cvec_free(nsctx); + if ((x = clicon_conf_xml(h)) != NULL) + xml_free(x); + xpath_optimize_exit(); + clixon_event_exit(); + clicon_handle_exit(h); + clixon_err_exit(); + clicon_log_exit(); + if (pidfile) + unlink(pidfile); + return 0; +} + +/*! Get which sockets are used from SNMP API, the register single sockets into clixon event system + * + * @param[in] h Clixon handle + * @param[in] register if 1 register snmp sockets with event handler. If 0 close and unregister + * This is a workaround for netsnmps API usiing fdset:s, instead an fdset is created before calling + * the snmp api + * if you use select(), see snmp_select_info() in snmp_api(3) + * snmp_select_info(int *numfds, fd_set *fdset, struct timeval *timeout, int *block) + * @see clixon_snmp_input_cb + */ +static int +clixon_snmp_fdset_register(clicon_handle h, + int regfd) +{ + int retval = -1; + int numfds = 0; + fd_set readfds; + struct timeval timeout = { LONG_MAX, 0 }; + int block = 0; + int nr; + int s; + + FD_ZERO(&readfds); + if ((nr = snmp_sess_select_info(NULL, &numfds, &readfds, &timeout, &block)) < 0){ + clicon_err(OE_XML, errno, "snmp_select_error"); + goto done; + } + /* eg 4, 6, 8 */ + for (s=0; smyvoid =(void*)sh; + /* Register our application data and how to free it */ + handler->myvoid = (void*)sh; + handler->data_clone = snmp_handle_clone; + handler->data_free = snmp_handle_free; + /* * XXX: nhreg->agent_data */ @@ -231,7 +235,7 @@ mibyang_table_register(clicon_handle h, clixon_snmp_handle *sh; int ret; netsnmp_mib_handler *handler; - netsnmp_table_registration_info *table_info=NULL; + netsnmp_table_registration_info *table_info = NULL; cvec *cvk = NULL; /* vector of index keys */ cg_var *cvi; char *keyname; @@ -274,7 +278,11 @@ mibyang_table_register(clicon_handle h, netsnmp_handler_free(handler); goto done; } + /* Register our application data and how to free it */ handler->myvoid =(void*)sh; + handler->data_clone = snmp_handle_clone; + handler->data_free = snmp_handle_free; + /* See netsnmp_register_table_data_set */ if ((table_info = SNMP_MALLOC_TYPEDEF(netsnmp_table_registration_info)) == NULL){ clicon_err(OE_UNIX, errno, "SNMP_MALLOC_TYPEDEF"); @@ -320,6 +328,7 @@ mibyang_table_register(clicon_handle h, clicon_err(OE_SNMP, ret, "netsnmp_register_table"); goto done; } + sh->sh_table_info = table_info; clicon_debug(1, "%s %s registered", __FUNCTION__, oidstr); ok: retval = 0; @@ -346,7 +355,7 @@ mibyang_table_traverse_static(clicon_handle h, { int retval = -1; cvec *nsc = NULL; - char *xpath; + char *xpath = NULL; cxobj *xt = NULL; cxobj *xerr; cxobj *xtable; @@ -408,6 +417,8 @@ mibyang_table_traverse_static(clicon_handle h, } retval = 0; done: + if (xpath) + free(xpath); if (cvk) cvec_free(cvk); if (xt) diff --git a/test/lib.sh b/test/lib.sh index 6b3c8e98..fa05286e 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -190,6 +190,9 @@ BUSER=clicon : ${clixon_snmp_pidfile:="/var/tmp/clixon_snmp.pid"} +# Temporary debug var, set to trigger remaining snmp errors +: ${snmp_debug:=false} + # Source the site-specific definitions for test script variables, if site.sh # exists. The variables defined in site.sh override any variables of the same # names in the environment in the current execution. diff --git a/test/test_snmp_get.sh b/test/test_snmp_get.sh index ff873060..694cb953 100755 --- a/test/test_snmp_get.sh +++ b/test/test_snmp_get.sh @@ -250,7 +250,8 @@ validate_oid $NAME12 $NAME12 "INTEGER" 1 new "Get bulk OIDs" expectpart "$($snmpbulkget $OID1)" 0 "$OID2 = INTEGER: -1" "$OID3 = STRING: \"This is not default\"" "$OID4 = Timeticks: (12345) 0:02:03.45" "$OID5 = INTEGER: 48" "$OID6 = Gauge32: 123123123" "$OID7 = INTEGER: 3" "$OID8 = Counter32: 123456" "$OID9 = Counter64: 4294967296" "$OID10 = INTEGER: 1" "$OID11 = Timeticks: (1234567890) 142 days, 21:21:18.90" -if [ $snmp_debug ]; then +snmp_debug=false +if $snmp_debug; then new "Test SNMP table netSnmpIETFWGTable" expectpart "$($snmptable $OID13)" 0 "Name1" "Name2"