From 2e238566761d61696cdc9d466d518567a5c5d44f Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sat, 22 Aug 2020 18:02:08 +0200 Subject: [PATCH] fixed memory leaks in restconf evhtp module --- apps/restconf/README.md | 32 ++++++++++- apps/restconf/restconf_api_evhtp.c | 1 - apps/restconf/restconf_main_evhtp.c | 89 +++++++++++++++++------------ lib/src/clixon_yang_parse_lib.c | 6 +- 4 files changed, 83 insertions(+), 45 deletions(-) diff --git a/apps/restconf/README.md b/apps/restconf/README.md index 292b5da1..79a88c1c 100644 --- a/apps/restconf/README.md +++ b/apps/restconf/README.md @@ -1,13 +1,37 @@ # Clixon Restconf - * [Installation](#installation) + * [Evhtp](#evhtp) + * [Nginx](#nginx) * [Streams](#streams) * [Nchan Streams](#nchan) * [Debugging](#debugging) -## Installation +There are two installation instructions: for libevhtp and nginx. -The examples are based on Nginx. Other reverse proxies should work but are not verified. +## Evhtp + +Download, build and install libevhtp from source. Prereqs: libevent and cmake. +``` + git clone https://github.com/criticalstack/libevhtp.git + cd libevhtp/build + cmake -DEVHTP_DISABLE_REGEX=ON -DEVHTP_DISABLE_EVTHR=ON .. + make + sudo make install +``` + +Configure clixon with evhtp: +``` + ./configure --with-restconf=evhtp +``` + +Ensure www-data is member of the CLICON_SOCK_GROUP (default clicon). If not, add it: +``` + sudo usermod -a -G clicon www-data +``` + +## Nginx + +Installation instruction for Nginx. Other reverse proxies should work but are not verified. Ensure www-data is member of the CLICON_SOCK_GROUP (default clicon). If not, add it: ``` @@ -49,6 +73,8 @@ Or on FreeBSD: sudo service nginx start ``` +## Run + Start clixon backend daemon (if not already started) ``` sudo clixon_backend -s init -f /usr/local/etc/example.xml diff --git a/apps/restconf/restconf_api_evhtp.c b/apps/restconf/restconf_api_evhtp.c index 289036c2..c300a991 100644 --- a/apps/restconf/restconf_api_evhtp.c +++ b/apps/restconf/restconf_api_evhtp.c @@ -118,7 +118,6 @@ restconf_reply_header(void *req0, clicon_err(OE_CFG, errno, "evhttp_header_new"); goto done; } - value = NULL; /* freed by evhtp */ evhtp_headers_add_header(req->headers_out, evhdr); retval = 0; done: diff --git a/apps/restconf/restconf_main_evhtp.c b/apps/restconf/restconf_main_evhtp.c index 25ece580..d6cebe03 100644 --- a/apps/restconf/restconf_main_evhtp.c +++ b/apps/restconf/restconf_main_evhtp.c @@ -38,11 +38,12 @@ #ifdef HAVE_CONFIG_H #include "clixon_config.h" /* generated by config & autoconf */ #endif -/* compilation withotu threading support - * XXX: could be disabled already in configure? + +/* The clixon evhtp code can be compiled with or without threading support + * The choice is set at libevhtp compile time by cmake. Eg: + * cmake -DEVHTP_DISABLE_EVTHR=ON # Disable threads. + * Default in testing is disabled threads. */ -//#define EVHTP_DISABLE_EVTHR -#define EVHTP_DISABLE_REGEX #include #include @@ -87,7 +88,24 @@ /* Need global variable to for signal handler XXX */ static clicon_handle _CLICON_HANDLE = NULL; +static struct evhtp_handle{ + evhtp_t *eh_htp; + struct event_base *eh_evbase; + evhtp_ssl_cfg_t *eh_ssl_config; +} _EVHTP_HANDLE = {0,}; +static void +evhtp_terminate(struct evhtp_handle *eh) +{ + if (eh->eh_htp){ + evhtp_unbind_socket(eh->eh_htp); + evhtp_free(eh->eh_htp); + } + if (eh->eh_evbase) + event_base_free(eh->eh_evbase); + if (eh->eh_ssl_config) + free(eh->eh_ssl_config); +} /*! Signall terminates process */ @@ -101,6 +119,7 @@ restconf_sig_term(int arg) __PROGRAM__, __FUNCTION__, getpid(), arg); else exit(-1); + evhtp_terminate(&_EVHTP_HANDLE); if (_CLICON_HANDLE){ // stream_child_freeall(_CLICON_HANDLE); restconf_terminate(_CLICON_HANDLE); @@ -457,6 +476,8 @@ cx_path_restconf(evhtp_request_t *req, if (restconf_param_del_all(h) < 0) goto done; done: + if (qvec) + cvec_free(qvec); return; /* void */ } @@ -601,15 +622,13 @@ main(int argc, size_t cligen_bufthreshold; uint16_t defaultport; uint16_t port = 0; - evhtp_t *htp = NULL; - struct event_base *evbase = NULL; - evhtp_ssl_cfg_t *ssl_config = NULL; int dbg = 0; int use_ssl = 0; int ssl_verify_clients = 0; char *restconf_ipv4_addr = NULL; char *restconf_ipv6_addr = NULL; int i; + struct evhtp_handle *eh = &_EVHTP_HANDLE; /* In the startup, logs to stderr & debug flag set later */ clicon_log_init(__PROGRAM__, LOG_INFO, logdst); @@ -747,20 +766,20 @@ main(int argc, /* Check server ssl certs */ if (use_ssl){ /* Init evhtp ssl config struct */ - if ((ssl_config = malloc(sizeof(evhtp_ssl_cfg_t))) == NULL){ + if ((eh->eh_ssl_config = malloc(sizeof(evhtp_ssl_cfg_t))) == NULL){ clicon_err(OE_UNIX, errno, "malloc"); goto done; } - memset(ssl_config, 0, sizeof(evhtp_ssl_cfg_t)); - ssl_config->ssl_opts = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | SSL_OP_NO_TLSv1; + memset(eh->eh_ssl_config, 0, sizeof(evhtp_ssl_cfg_t)); + eh->eh_ssl_config->ssl_opts = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | SSL_OP_NO_TLSv1; - if (cx_get_certs(h, ssl_verify_clients, ssl_config) < 0) + if (cx_get_certs(h, ssl_verify_clients, eh->eh_ssl_config) < 0) goto done; - ssl_config->x509_verify_cb = cx_verify_certs; /* Is extra verification necessary? */ + eh->eh_ssl_config->x509_verify_cb = cx_verify_certs; /* Is extra verification necessary? */ if (ssl_verify_clients){ - ssl_config->verify_peer = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT; - ssl_config->x509_verify_cb = cx_verify_certs; - ssl_config->verify_depth = 2; + eh->eh_ssl_config->verify_peer = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT; + eh->eh_ssl_config->x509_verify_cb = cx_verify_certs; + eh->eh_ssl_config->verify_depth = 2; } } @@ -770,44 +789,44 @@ main(int argc, clicon_argv_set(h, argv0, argc, argv); /* Init evhtp */ - if ((evbase = event_base_new()) == NULL){ + if ((eh->eh_evbase = event_base_new()) == NULL){ clicon_err(OE_UNIX, errno, "event_base_new"); goto done; } /* create a new evhtp_t instance */ - if ((htp = evhtp_new(evbase, NULL)) == NULL){ + if ((eh->eh_htp = evhtp_new(eh->eh_evbase, NULL)) == NULL){ clicon_err(OE_UNIX, errno, "evhtp_new"); goto done; } /* Here the daemon either uses SSL or not, ie you cant seem to mix http and https :-( */ if (use_ssl){ - if (evhtp_ssl_init(htp, ssl_config) < 0){ + if (evhtp_ssl_init(eh->eh_htp, eh->eh_ssl_config) < 0){ clicon_err(OE_UNIX, errno, "evhtp_new"); goto done; } } #ifndef EVHTP_DISABLE_EVTHR - evhtp_use_threads_wexit(htp, NULL, NULL, 4, NULL); + evhtp_use_threads_wexit(eh->eh_htp, NULL, NULL, 4, NULL); #endif /* Callback before the connection is accepted. */ - evhtp_set_pre_accept_cb(htp, cx_pre_accept, h); + evhtp_set_pre_accept_cb(eh->eh_htp, cx_pre_accept, h); /* Callback right after a connection is accepted. */ - evhtp_set_post_accept_cb(htp, cx_post_accept, h); + evhtp_set_post_accept_cb(eh->eh_htp, cx_post_accept, h); /* Callback to be executed for all /restconf api calls */ - if (evhtp_set_cb(htp, "/" RESTCONF_API, cx_path_restconf, h) == NULL){ + if (evhtp_set_cb(eh->eh_htp, "/" RESTCONF_API, cx_path_restconf, h) == NULL){ clicon_err(OE_EVENTS, errno, "evhtp_set_cb"); goto done; } /* Callback to be executed for all /restconf api calls */ - if (evhtp_set_cb(htp, RESTCONF_WELL_KNOWN, cx_path_wellknown, h) == NULL){ + if (evhtp_set_cb(eh->eh_htp, RESTCONF_WELL_KNOWN, cx_path_wellknown, h) == NULL){ clicon_err(OE_EVENTS, errno, "evhtp_set_cb"); goto done; } /* Generic callback called if no other callbacks are matched */ - evhtp_set_gencb(htp, cx_gencb, h); + evhtp_set_gencb(eh->eh_htp, cx_gencb, h); /* bind to a socket, optionally with specific protocol support formatting */ @@ -825,7 +844,7 @@ main(int argc, goto done; } cprintf(cb, "ipv4:%s", restconf_ipv4_addr); - if (evhtp_bind_socket(htp, /* evhtp handle */ + if (evhtp_bind_socket(eh->eh_htp, /* evhtp handle */ cbuf_get(cb), /* string address, eg ipv4: */ port, /* port */ SOCKET_LISTEN_BACKLOG /* backlog flag, see listen(5) */ @@ -836,14 +855,15 @@ main(int argc, if (cb) cbuf_free(cb); } - if (restconf_ipv6_addr != NULL && strlen(restconf_ipv6_addr)){ + /* Eeh can only bind one */ + if (0 && restconf_ipv6_addr != NULL && strlen(restconf_ipv6_addr)){ cbuf *cb; if ((cb = cbuf_new()) == NULL){ clicon_err(OE_UNIX, errno, "cbuf_new"); goto done; } cprintf(cb, "ipv6:%s", restconf_ipv6_addr); - if (evhtp_bind_socket(htp, /* evhtp handle */ + if (evhtp_bind_socket(eh->eh_htp, /* evhtp handle */ cbuf_get(cb), /* string address, eg ipv6: */ port, /* port */ SOCKET_LISTEN_BACKLOG /* backlog flag, see listen(5) */ @@ -855,7 +875,7 @@ main(int argc, cbuf_free(cb); } /* Drop privileges to WWWUSER if started as root */ - if (restconf_drop_privileges(h, WWWUSER) < 0) + if (0 && restconf_drop_privileges(h, WWWUSER) < 0) goto done; /* Init cligen buffers */ @@ -944,20 +964,13 @@ main(int argc, if (clixon_plugin_start_all(h) < 0) goto done; - /* Find and read configfile */ - if (clicon_options_main(h) < 0) - goto done; - - event_base_loop(evbase, 0); - - evhtp_unbind_socket(htp); - - // evhtp_safe_free(htp, evhtp_free); - // evhtp_safe_free(evbase, event_base_free); + event_base_loop(eh->eh_evbase, 0); retval = 0; done: + clicon_debug(1, "restconf_main_evhtp done"); // stream_child_freeall(h); + evhtp_terminate(&_EVHTP_HANDLE); restconf_terminate(h); return retval; } diff --git a/lib/src/clixon_yang_parse_lib.c b/lib/src/clixon_yang_parse_lib.c index 92907a66..50b98c49 100644 --- a/lib/src/clixon_yang_parse_lib.c +++ b/lib/src/clixon_yang_parse_lib.c @@ -554,8 +554,8 @@ yang_parse_file(int fd, len = BUFLEN; /* any number is fine */ if ((buf = malloc(len)) == NULL){ - perror("pt_file malloc"); - return NULL; + clicon_err(OE_XML, errno, "malloc"); + goto done; } memset(buf, 0, len); i = 0; /* position in buf */ @@ -579,7 +579,7 @@ yang_parse_file(int fd, if ((ymod = yang_parse_str(buf, name, yspec)) < 0) goto done; done: - if (buf) + if (buf != NULL) free(buf); return ymod; /* top-level (sub)module */ }