From 8cde7a4deda943d39196c64ffbd15d4441cbf7c9 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Thu, 7 Jan 2021 16:57:47 +0100 Subject: [PATCH] * Handling empty netconf XML messages "]]>]]>" is changed from being accepted to return an error. * Netconf split lines input (input fragments) fixed * Netconf input split on several lines, eg using stdin: "\nfoo]]>]]>" could under some circumstances be split so that only "]]>]]>" be properly processed. This could also happen to a socket receiving a sub-string and then after a delay receive the rest. * Fixed by storing residue and add that to the input string if later input is received on the same socket. * Use [https://github.com/clicon/libevhtp](https://github.com/clicon/libevhtp) instead of [https://github.com/criticalstack/libevhtp](https://github.com/criticalstack/libevhtp) as a source of the evhtp source --- CHANGELOG.md | 7 ++++- apps/netconf/netconf_main.c | 51 ++++++++++++++++++++++++++++++++---- apps/restconf/README.md | 2 +- configure | 2 +- configure.ac | 2 +- docker/base/Dockerfile | 2 +- docker/main/Dockerfile.evhtp | 2 +- lib/src/clixon_xml_io.c | 11 +++++++- test/test_netconf.sh | 2 +- test/travis/build_evhtp.sh | 2 +- test/vagrant/vagrant.sh | 2 +- 11 files changed, 70 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e435e252..69ee681e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Clixon Changelog -* [4.10.0](#4100) Expected: February +* [4.10.0](#4100) Expected: February 2021 * [4.9.0](#490) 18 December 2020 * [4.8.0](#480) 18 October 2020 * [4.7.0](#470) 14 September 2020 @@ -39,6 +39,7 @@ Developers may need to change their code Users may have to change how they access the system +* Handling empty netconf XML messages "]]>]]>" is changed from being accepted to return an error. * New clixon-lib@2020-12-30.yang revision * Changed: RPC process-control output parameter status to pid * New clixon-config@2020-12-30.yang revision @@ -49,6 +50,7 @@ Users may have to change how they access the system ### Minor changes +* Use [https://github.com/clicon/libevhtp](https://github.com/clicon/libevhtp) instead of [https://github.com/criticalstack/libevhtp](https://github.com/criticalstack/libevhtp) as a source of the evhtp source * Added callback to process-control RPC feature in clixon-lib.yang to manage processes * WHen an RPC comes in, be able to look at configuration * Changed behavior of starting restconf internally using `CLICON_BACKEND_RESTCONF_PROCESS` monitoring changes in enable flag, not only the RPC. The semantics is as follows: @@ -61,6 +63,9 @@ Users may have to change how they access the system ### Corrected Bugs +* Netconf split lines input (input fragments) fixed + * Netconf input split on several lines, eg using stdin: "\nfoo]]>]]>" could under some circumstances be split so that only "]]>]]>" be properly processed. This could also happen to a socket receiving a sub-string and then after a delay receive the rest. + * Fixed by storing residue and add that to the input string if later input is received on the same socket. * [Presence container configs not displayed in 'show config set' #164 ](https://github.com/clicon/clixon/issues/164) * Treat presence container as a leaf: always print a placeholder regardless if it has children or not. An extra check for children could have been made to not print if it has, but this adds an extra minor complexity. diff --git a/apps/netconf/netconf_main.c b/apps/netconf/netconf_main.c index 7b26968e..298d0163 100644 --- a/apps/netconf/netconf_main.c +++ b/apps/netconf/netconf_main.c @@ -75,6 +75,12 @@ #define NETCONF_LOGFILE "/tmp/clixon_netconf.log" +/* clixon-data value to save buffer between invocations. + * Saving data may be necessary if socket buffer contains partial netconf messages, such as: + * ..wait 1min ]]>]]> + */ +#define NETCONF_HASH_BUF "netconf_input_cbuf" + /*! Ignore errors on packet errors: continue */ static int ignore_packet_errors = 1; @@ -265,6 +271,17 @@ netconf_input_frame(clicon_handle h, clicon_err(OE_UNIX, errno, "strdup"); goto done; } + /* Special case: */ + if (strlen(str) == 0){ + if ((cbret = cbuf_new()) == NULL){ + clicon_err(OE_UNIX, errno, "cbuf_new"); + goto done; + } + if (netconf_operation_failed(cbret, "rpc", "Empty XML")< 0) + goto done; + netconf_output_encap(1, cbret, "rpc-error"); + goto ok; + } /* Parse incoming XML message */ if ((ret = clixon_xml_parse_string(str, YB_RPC, yspec, &xtop, &xret)) < 0){ if ((cbret = cbuf_new()) == NULL){ @@ -331,6 +348,10 @@ netconf_input_frame(clicon_handle h, * This routine continuously reads until no more data on s. There could * be risk of starvation, but the netconf client does little else than * read data so I do not see a danger of true starvation here. + * @note data is saved in clicon-handle at NETCONF_HASH_BUF since there is a potential issue if data + * is not completely present on the s, ie if eg: + * foo ..pause.. ]]>]]> + * then only "" would be delivered to netconf_input_frame(). */ static int netconf_input_cb(int s, @@ -344,10 +365,23 @@ netconf_input_cb(int s, cbuf *cb=NULL; int xml_state = 0; int poll; + clicon_hash_t *cdat = clicon_data(h); /* Save cbuf between calls if not done */ + size_t cdatlen = 0; + void *ptr; - if ((cb = cbuf_new()) == NULL){ - clicon_err(OE_XML, errno, "cbuf_new"); - return retval; + if ((ptr = clicon_hash_value(cdat, NETCONF_HASH_BUF, &cdatlen)) != NULL){ + if (cdatlen != sizeof(cb)){ + clicon_err(OE_XML, errno, "size mismatch %lu %lu", cdatlen, sizeof(cb)); + goto done; + } + cb = *(cbuf**)ptr; + clicon_hash_del(cdat, NETCONF_HASH_BUF); + } + else{ + if ((cb = cbuf_new()) == NULL){ + clicon_err(OE_XML, errno, "cbuf_new"); + goto done; + } } memset(buf, 0, sizeof(buf)); while (1){ @@ -386,8 +420,15 @@ netconf_input_cb(int s, /* poll==1 if more, poll==0 if none */ if ((poll = clixon_event_poll(s)) < 0) goto done; - if (poll == 0) - break; /* No data to read */ + if (poll == 0){ + /* No data to read, save data and continue on next round */ + if (cbuf_len(cb) != 0){ + if (clicon_hash_add(cdat, NETCONF_HASH_BUF, &cb, sizeof(cb)) == NULL) + return -1; + cb = NULL; + } + break; + } } /* while */ retval = 0; done: diff --git a/apps/restconf/README.md b/apps/restconf/README.md index 79a88c1c..482670e3 100644 --- a/apps/restconf/README.md +++ b/apps/restconf/README.md @@ -12,7 +12,7 @@ There are two installation instructions: for libevhtp and nginx. Download, build and install libevhtp from source. Prereqs: libevent and cmake. ``` - git clone https://github.com/criticalstack/libevhtp.git + git clone https://github.com/clicon/libevhtp.git cd libevhtp/build cmake -DEVHTP_DISABLE_REGEX=ON -DEVHTP_DISABLE_EVTHR=ON .. make diff --git a/configure b/configure index 4b81ef98..af03d6d2 100755 --- a/configure +++ b/configure @@ -5053,7 +5053,7 @@ if test "x$ac_cv_header_evhtp_evhtp_h" = xyes; then : _ACEOF else - as_fn_error $? "evhtp header missing. See https://github.com/criticalstack/libevhtp" "$LINENO" 5 + as_fn_error $? "evhtp header missing. See https://github.com/clicon/libevhtp" "$LINENO" 5 fi done diff --git a/configure.ac b/configure.ac index 9f44a294..d67616f8 100644 --- a/configure.ac +++ b/configure.ac @@ -211,7 +211,7 @@ if test "x${with_restconf}" == xfcgi; then # Lives in libfcgi-dev AC_CHECK_LIB(fcgi, FCGX_Init,, AC_MSG_ERROR([libfcgi-dev missing])) elif test "x${with_restconf}" == xevhtp; then - AC_CHECK_HEADERS(evhtp/evhtp.h,, AC_MSG_ERROR([evhtp header missing. See https://github.com/criticalstack/libevhtp])) + AC_CHECK_HEADERS(evhtp/evhtp.h,, AC_MSG_ERROR([evhtp header missing. See https://github.com/clicon/libevhtp])) #LIBS += -lpthread -levent -levent_openssl -lssl -lcrypto AC_CHECK_LIB(pthread, pthread_create,, AC_MSG_ERROR([libpthread missing])) AC_CHECK_LIB(event, event_init,, AC_MSG_ERROR([libevent missing])) diff --git a/docker/base/Dockerfile b/docker/base/Dockerfile index be8d3792..51a294d1 100644 --- a/docker/base/Dockerfile +++ b/docker/base/Dockerfile @@ -47,7 +47,7 @@ RUN apk add --update libevent cmake libevent-dev # clone libevhtp WORKDIR /clixon -RUN git clone https://github.com/criticalstack/libevhtp.git +RUN git clone https://github.com/clicon/libevhtp.git WORKDIR /clixon/libevhtp/build RUN cmake -DEVHTP_DISABLE_REGEX=ON -DEVHTP_DISABLE_EVTHR=ON .. RUN make diff --git a/docker/main/Dockerfile.evhtp b/docker/main/Dockerfile.evhtp index 4bf23ef1..7ae1ba2a 100644 --- a/docker/main/Dockerfile.evhtp +++ b/docker/main/Dockerfile.evhtp @@ -47,7 +47,7 @@ RUN apk add --update libevent cmake libevent-dev # clone libevhtp WORKDIR /clixon -RUN git clone https://github.com/criticalstack/libevhtp.git +RUN git clone https://github.com/clicon/libevhtp.git WORKDIR /clixon/libevhtp/build RUN cmake -DEVHTP_DISABLE_REGEX=ON -DEVHTP_DISABLE_EVTHR=ON -DBUILD_SHARED_LIBS=OFF .. RUN make diff --git a/lib/src/clixon_xml_io.c b/lib/src/clixon_xml_io.c index 203c51a1..a4c24228 100644 --- a/lib/src/clixon_xml_io.c +++ b/lib/src/clixon_xml_io.c @@ -444,6 +444,14 @@ xmltree2cbuf(cbuf *cb, * @see clixon_xml_parse_string * @see _json_parse * @note special case is empty XML where the parser is not invoked. + * It is questionable empty XML is legal. From https://www.w3.org/TR/2008/REC-xml-20081126 Sec 2.1: + * A well-formed document ... contains one or more elements. + * But in clixon one can invoke a parser on a sub-part of a document where it makes sense to accept + * an empty XML. For example where an empty config: is parsed. + * In other cases, such as receiving netconf ]]>]]> it should represent a complete document and + * therefore not well-formed. + * Therefore checking for empty XML must be done by a calling function which knows wether the + * the XML represents a full document or not. */ static int _xml_parse(const char *str, @@ -460,8 +468,9 @@ _xml_parse(const char *str, int i; clicon_debug(2, "%s", __FUNCTION__); - if (strlen(str) == 0) + if (strlen(str) == 0){ return 1; /* OK */ + } if (xt == NULL){ clicon_err(OE_XML, errno, "Unexpected NULL XML"); return -1; diff --git a/test/test_netconf.sh b/test/test_netconf.sh index 6c1ae11a..97184000 100755 --- a/test/test_netconf.sh +++ b/test/test_netconf.sh @@ -54,7 +54,7 @@ fi # Framing. with -q to inhibit rcv hello new "Empty frame" -expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' ']]>]]>' +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' 'rpcoperation-failederrorEmpty XML]]>]]>' if [ $valgrindtest -eq 0 ]; then # Some leakage in lex / error handling difficult to catch new "Frame invalid non-xml" diff --git a/test/travis/build_evhtp.sh b/test/travis/build_evhtp.sh index 2ddb3a5f..a5531d56 100755 --- a/test/travis/build_evhtp.sh +++ b/test/travis/build_evhtp.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash # Travis pre-config script. # build libevhtp -git clone https://github.com/criticalstack/libevhtp.git +git clone https://github.com/clicon/libevhtp.git (cd libevhtp/build && cmake -DEVHTP_DISABLE_REGEX=ON -DEVHTP_DISABLE_EVTHR=ON .. && make && sudo make install) diff --git a/test/vagrant/vagrant.sh b/test/vagrant/vagrant.sh index 7b65471e..1750620e 100755 --- a/test/vagrant/vagrant.sh +++ b/test/vagrant/vagrant.sh @@ -267,7 +267,7 @@ case ${with_restconf} in fi if $buildevhtp; then $sshcmd << 'EOF' - test -d libevhtp || sudo git clone https://github.com/criticalstack/libevhtp.git + test -d libevhtp || sudo git clone https://github.com/clicon/libevhtp.git cd libevhtp/build; CMAKE=$(which cmake) sudo $CMAKE -DEVHTP_DISABLE_REGEX=ON -DEVHTP_DISABLE_EVTHR=ON -DBUILD_SHARED_LIBS=OFF ..