From 2def5d2367ae6d1aa2c05d516dfd31e66041f273 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Thu, 1 Apr 2021 12:41:36 +0200 Subject: [PATCH] * Fixed Yang parsing of comments in (extension) unknown statements, to allow multiple white space * Fixed cli_start_shell: comments and cli_error * Fixed [making cli_show_options's output more human readable #199](https://github.com/clicon/clixon/issues/199) * Fixed SSL/evhtp limited read buffer problem --- CHANGELOG.md | 3 + apps/cli/cli_common.c | 17 +-- apps/cli/cli_show.c | 2 +- apps/netconf/netconf_main.c | 2 +- apps/restconf/restconf_main_evhtp.c | 181 +++++++++++++++++----------- example/main/example_cli.cli | 1 + lib/src/clixon_yang_parse.l | 3 +- lib/src/clixon_yang_parse.y | 22 +++- 8 files changed, 142 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b0bd5f2..dd3d027e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,9 @@ Developers may need to change their code ### Corrected Bugs +* Fixed [making cli_show_options's output more human readable #199](https://github.com/clicon/clixon/issues/199) +* Fixed Yang parsing of comments in (extension) unknown statements, to allow multiple white space + * this also caused spaces to be printed to stdout after clixon-restconf was terminated * Fixed: [clixon_restconf not properly configed and started by clixon_backend #193](clixon_restconf not properly configed and started by clixon_backend #193) * Fixed: [backend start resconf failed due to path string truncated #192](https://github.com/clicon/clixon/issues/192) * Fixed: [state showing error in cli with CLICON_STREAM_DISCOVERY_RFC8040 #191](https://github.com/clicon/clixon/issues/191) diff --git a/apps/cli/cli_common.c b/apps/cli/cli_common.c index 4825ff99..d773cb99 100644 --- a/apps/cli/cli_common.c +++ b/apps/cli/cli_common.c @@ -550,8 +550,7 @@ cli_set_mode(clicon_handle h, } /*! Start bash from cli callback - * XXX Application specific?? - * XXX replace fprintf with clicon_err? + * Typical usage: shell("System Bash") , cli_start_shell(); */ int cli_start_shell(clicon_handle h, @@ -567,13 +566,11 @@ cli_start_shell(clicon_handle h, cmd = (cvec_len(vars)>1 ? cv_string_get(cv1) : NULL); if ((pw = getpwuid(getuid())) == NULL){ - fprintf(stderr, "%s: getpwuid: %s\n", - __FUNCTION__, strerror(errno)); + clicon_err(OE_UNIX, errno, "getpwuid"); goto done; } if (chdir(pw->pw_dir) < 0){ - fprintf(stderr, "%s: chdir(%s): %s\n", - __FUNCTION__, pw->pw_dir, strerror(errno)); + clicon_err(OE_UNIX, errno, "chdir"); endpwent(); goto done; } @@ -584,22 +581,20 @@ cli_start_shell(clicon_handle h, snprintf(bcmd, 128, "bash -l -c \"%s\"", cmd); if (system(bcmd) < 0){ cli_signal_block(h); - fprintf(stderr, "%s: system(bash -c): %s\n", - __FUNCTION__, strerror(errno)); + clicon_err(OE_UNIX, errno, "system(bash -c)"); goto done; } } else if (system("bash -l") < 0){ cli_signal_block(h); - fprintf(stderr, "%s: system(bash): %s\n", - __FUNCTION__, strerror(errno)); + clicon_err(OE_UNIX, errno, "system(bash)"); goto done; } cli_signal_block(h); #if 0 /* Allow errcodes from bash */ if (retval != 0){ - fprintf(stderr, "%s: system(%s) code=%d\n", __FUNCTION__, cmd, retval); + clicon_err(OE_UNIX, errno, "system(%s) %d", cmd, retval); goto done; } #endif diff --git a/apps/cli/cli_show.c b/apps/cli/cli_show.c index d0d72473..f0a20904 100644 --- a/apps/cli/cli_show.c +++ b/apps/cli/cli_show.c @@ -877,7 +877,7 @@ cli_show_options(clicon_handle h, fprintf(stdout, "%s: 0x%p , length %zu\n", keys[i], val, vlen); } else - fprintf(stdout, "%s: NULL", keys[i]); + fprintf(stdout, "%s: NULL\n", keys[i]); } /* Next print CLICON_FEATURE and CLICON_YANG_DIR from config tree * Since they are lists they are placed in the config tree. diff --git a/apps/netconf/netconf_main.c b/apps/netconf/netconf_main.c index 94259b9f..70896242 100644 --- a/apps/netconf/netconf_main.c +++ b/apps/netconf/netconf_main.c @@ -464,7 +464,7 @@ netconf_input_cb(int s, { int retval = -1; clicon_handle h = arg; - unsigned char buf[BUFSIZ]; + unsigned char buf[BUFSIZ]; /* from stdio.h, typically 8K */ int i; int len; cbuf *cb=NULL; diff --git a/apps/restconf/restconf_main_evhtp.c b/apps/restconf/restconf_main_evhtp.c index be91ebae..e4654394 100644 --- a/apps/restconf/restconf_main_evhtp.c +++ b/apps/restconf/restconf_main_evhtp.c @@ -227,8 +227,28 @@ buf_write(char *buf, if (ssl){ if ((ret = SSL_write(ssl, buf, buflen)) <= 0){ - clicon_err(OE_SSL, 0, "SSL_write"); - goto done; + int e = errno; + switch (SSL_get_error(ssl, ret)){ + case SSL_ERROR_SYSCALL: /* 5 */ + if (e == ECONNRESET) {/* Connection reset by peer */ + clicon_debug(1, "%s HERE", __FUNCTION__); + if (ssl) + SSL_free(ssl); + close(s); + clixon_event_unreg_fd(s, restconf_connection); + goto ok; /* Close socket and ssl */ + } + else{ + clicon_err(OE_RESTCONF, e, "SSL_write %d", e); + goto done; + } + break; + default: + clicon_err(OE_SSL, 0, "SSL_write"); + goto done; + break; + } + goto done; } } else{ @@ -237,6 +257,7 @@ buf_write(char *buf, goto done; } } + ok: retval = 0; done: return retval; @@ -477,6 +498,7 @@ evhtp_params_set(clicon_handle h, goto done; retval = 1; done: + clicon_debug(1, "%s %d", __FUNCTION__, retval); if (subject) free(subject); if (cvv) @@ -542,6 +564,7 @@ restconf_path_root(evhtp_request_t *req, goto done; retval = 0; done: + clicon_debug(1, "%s %d", __FUNCTION__, retval); /* Catch all on fatal error. This does not terminate the process but closes request stream */ if (retval < 0){ evhtp_send_reply(req, EVHTP_RES_ERROR); @@ -791,7 +814,7 @@ close_openssl_socket(int s, * @param[in] ssl if set, it will be freed */ static int -accept_badrequest(clicon_handle h, +send_badrequest(clicon_handle h, int s, SSL *ssl) { @@ -814,6 +837,10 @@ accept_badrequest(clicon_handle h, * @retval -1 Error Terminates backend and is never called). Instead errors are * propagated back to client. * @see restconf_accept_client where this callback is registered + * @note read buffer is limited. More data can be read in two ways: either evhtp returns a buffer + * with 100 Continue, in which case that is replied and the function returns and the client sends + * more data. + * OR evhtp returns 0 with no reply, then this is assumed to mean read more data from the socket. */ static int restconf_connection(int s, @@ -822,9 +849,10 @@ restconf_connection(int s, int retval = -1; evhtp_connection_t *conn = NULL; ssize_t n; - char buf[1024]; + char buf[BUFSIZ]; /* from stdio.h, typically 8K */ SSL *ssl; clicon_handle h; + int readmore = 1; clicon_debug(1, "%s", __FUNCTION__); if ((conn = (evhtp_connection_t*)arg) == NULL){ @@ -832,74 +860,85 @@ restconf_connection(int s, goto done; } h = (clicon_handle*)conn->htp->arg; - /* Example: curl -Ssik -u wilma:bar -X GET https://localhost/restconf/data/example:x */ - if (conn->ssl){ - /* Non-ssl gets n == 0 here! - curl -Ssik --key /var/tmp/./test_restconf_ssl_certs.sh/certs/limited.key --cert /var/tmp/./test_restconf_ssl_certs.sh/certs/limited.crt -X GET https://localhost/restconf/data/example:x -*/ - if ((n = SSL_read(conn->ssl, buf, sizeof(buf))) < 0){ - clicon_err(OE_XML, errno, "SSL_read"); - goto done; - } - } - else{ - if ((n = read(conn->sock, buf, sizeof(buf))) < 0){ /* XXX atomicio ? */ - clicon_err(OE_XML, errno, "SSL_read"); - goto done; - } - } - if (n == 0){ - clicon_debug(1, "%s n=0 closing socket", __FUNCTION__); - ssl = conn->ssl; - conn->ssl = NULL; - evhtp_connection_free(conn); /* evhtp */ - if (close_openssl_socket(s, ssl) < 0) - goto done; - goto ok; - } - /* parse incoming packet - * signature: - */ - if (connection_parse_nobev(buf, n, conn) < 0){ - clicon_debug(1, "%s connection_parse error", __FUNCTION__); - if (accept_badrequest(h, s, conn->ssl) < 0) - goto done; - SSL_free(conn->ssl); - if (close_openssl_socket(s, NULL) < 0) - goto done; - conn->ssl = NULL; - evhtp_connection_free(conn); - goto ok; - } - if (conn->bev != NULL){ - char *buf = NULL; - size_t buflen; - struct evbuffer *ev; - - if ((ev = bufferevent_get_output(conn->bev)) != NULL){ - buf = (char*)evbuffer_pullup(ev, -1); - buflen = evbuffer_get_length(ev); - if (buflen){ - if (buf_write(buf, buflen, conn->sock, conn->ssl) < 0) - goto done; - } - else{ - clicon_debug(1, "%s bev is NULL 1", __FUNCTION__); - if (accept_badrequest(h, s, conn->ssl) < 0) /* actually error */ - goto done; + while (readmore) { + readmore = 0; + /* Example: curl -Ssik -u wilma:bar -X GET https://localhost/restconf/data/example:x */ + if (conn->ssl){ + /* Non-ssl gets n == 0 here! + curl -Ssik --key /var/tmp/./test_restconf_ssl_certs.sh/certs/limited.key --cert /var/tmp/./test_restconf_ssl_certs.sh/certs/limited.crt -X GET https://localhost/restconf/data/example:x + */ + if ((n = SSL_read(conn->ssl, buf, sizeof(buf))) < 0){ + clicon_err(OE_XML, errno, "SSL_read"); + goto done; } } else{ - clicon_debug(1, "%s bev is NULL 2", __FUNCTION__); - if (accept_badrequest(h, s, conn->ssl) < 0) /* actually error */ + if ((n = read(conn->sock, buf, sizeof(buf))) < 0){ /* XXX atomicio ? */ + clicon_err(OE_XML, errno, "SSL_read"); goto done; - } - } - else{ - clicon_debug(1, "%s bev is NULL 3", __FUNCTION__); - if (accept_badrequest(h, s, conn->ssl) < 0) /* actually error */ - goto done; - } + } + } + if (n == 0){ + clicon_debug(1, "%s n=0 closing socket", __FUNCTION__); + ssl = conn->ssl; + conn->ssl = NULL; + evhtp_connection_free(conn); /* evhtp */ + if (close_openssl_socket(s, ssl) < 0) + goto done; + goto ok; + } + /* parse incoming packet + * signature: + */ + if (connection_parse_nobev(buf, n, conn) < 0){ + clicon_debug(1, "%s connection_parse error", __FUNCTION__); + if (send_badrequest(h, s, conn->ssl) < 0) + goto done; + SSL_free(conn->ssl); + if (close_openssl_socket(s, NULL) < 0) + goto done; + conn->ssl = NULL; + evhtp_connection_free(conn); + goto ok; + } + clicon_debug(1, "%s connection_parse OK", __FUNCTION__); + if (conn->bev != NULL){ + char *buf = NULL; + size_t buflen; + struct evbuffer *ev; + + if ((ev = bufferevent_get_output(conn->bev)) != NULL){ + buf = (char*)evbuffer_pullup(ev, -1); + buflen = evbuffer_get_length(ev); + clicon_debug(1, "buf:%s", buf); + if (buflen){ + if (buf_write(buf, buflen, conn->sock, conn->ssl) < 0) + goto done; + } + else{ +#if 1 + /* Return 0 from evhtp parser can be that it needs more data. + */ + readmore = 1; /* Readmore */ +#else + clicon_debug(1, "%s bev is NULL 1", __FUNCTION__); + if (send_badrequest(h, s, conn->ssl) < 0) /* actually error */ + goto done; +#endif + } + } + else{ + clicon_debug(1, "%s bev is NULL 2", __FUNCTION__); + if (send_badrequest(h, s, conn->ssl) < 0) /* actually error */ + goto done; + } + } + else{ + clicon_debug(1, "%s bev is NULL 3", __FUNCTION__); + if (send_badrequest(h, s, conn->ssl) < 0) /* actually error */ + goto done; + } + } /* while moredata */ ok: retval = 0; done: @@ -1057,7 +1096,7 @@ restconf_accept_client(int fd, switch (e){ case SSL_ERROR_SSL: /* 1 */ clicon_debug(1, "%s SSL_ERROR_SSL (not ssl mesage on ssl socket)", __FUNCTION__); - if (accept_badrequest(h, s, NULL) < 0) + if (send_badrequest(h, s, NULL) < 0) goto done; SSL_free(ssl); if (close_openssl_socket(s, NULL) < 0) @@ -1089,6 +1128,8 @@ restconf_accept_client(int fd, case SSL_ERROR_WANT_CLIENT_HELLO_CB: /* 11 */ #endif break; + default: + break; } clicon_err(OE_SSL, 0, "SSL_accept:%d", e); goto done; @@ -1100,7 +1141,7 @@ restconf_accept_client(int fd, */ if (restconf_auth_type_get(h) == CLIXON_AUTH_CLIENT_CERTIFICATE && SSL_get_peer_certificate(ssl) == NULL) { /* Get certificates (if available) */ - if (accept_badrequest(h, s, ssl) < 0) + if (send_badrequest(h, s, ssl) < 0) goto done; if (ssl){ if ((ret = SSL_shutdown(ssl)) < 0){ diff --git a/example/main/example_cli.cli b/example/main/example_cli.cli index 05ea7697..698b59dc 100644 --- a/example/main/example_cli.cli +++ b/example/main/example_cli.cli @@ -56,6 +56,7 @@ no("Negate or remove") debug("Debugging parts of the system"), cli_debug_cli((in debug("Debugging parts of the system"), cli_debug_cli((int32)1);{ level("Set debug level: 1..n") ("Set debug level (0..n)"), cli_debug_backend(); } +shell("System command") , cli_start_shell(); copy("Copy and create a new object") { running("Copy from running db") startup("Copy to startup config"), db_copy("running", "startup"); interface("Copy interface"){ diff --git a/lib/src/clixon_yang_parse.l b/lib/src/clixon_yang_parse.l index d542061c..fd84265c 100644 --- a/lib/src/clixon_yang_parse.l +++ b/lib/src/clixon_yang_parse.l @@ -200,7 +200,7 @@ identifier [A-Za-z_][A-Za-z0-9_\-\.]* : { return *yytext; } ; { BEGIN(KEYWORD); return *yytext; } \{ { BEGIN(KEYWORD); return *yytext; } -[ \t\n]+ { BEGIN(UNKNOWN2);return SEP; } +[ \t\n]+ { BEGIN(UNKNOWN2); return WS; /* mandatory sep for string */ } [^{"';: \t\n]+ { clixon_yang_parselval.string = strdup(yytext); return CHARS; } @@ -208,6 +208,7 @@ identifier [A-Za-z_][A-Za-z0-9_\-\.]* \" { _YY->yy_lex_string_state =STRING; BEGIN(STRINGDQ); return *yytext; } \' { _YY->yy_lex_string_state =STRING; BEGIN(STRINGSQ); return SQ; } \{ { BEGIN(KEYWORD); return *yytext; } +[ \t\n]+ { return WS; } [^{"'; \t\n]+ { clixon_yang_parselval.string = strdup(yytext); return CHARS; } diff --git a/lib/src/clixon_yang_parse.y b/lib/src/clixon_yang_parse.y index 4cd589e0..ad171346 100644 --- a/lib/src/clixon_yang_parse.y +++ b/lib/src/clixon_yang_parse.y @@ -57,8 +57,9 @@ %token MY_EOF %token SQ /* Single quote: ' */ -%token SEP /* Separators (at least one) */ +%token WS /* white space (at least one) */ %token CHARS +%token ERRCHARS /* Error chars */ %token IDENTIFIER %token BOOL %token INT @@ -1574,10 +1575,10 @@ unknown_stmt : ustring ':' ustring optsep ';' if (ysp_add(_yy, Y_UNKNOWN, id, NULL) == NULL) _YYERROR("unknown_stmt"); _PARSE_DEBUG("unknown-stmt -> ustring : ustring ;"); } - | ustring ':' ustring SEP string optsep ';' + | ustring ':' ustring sep string optsep ';' { char *id; if ((id=string_del_join($1, ":", $3)) == NULL) _YYERROR("unknown_stmt"); if (ysp_add(_yy, Y_UNKNOWN, id, $5) == NULL){ _YYERROR("unknown_stmt"); } - _PARSE_DEBUG("unknown-stmt -> ustring : ustring SEP string ;"); + _PARSE_DEBUG("unknown-stmt -> ustring : ustring sep string ;"); } | ustring ':' ustring optsep { char *id; if ((id=string_del_join($1, ":", $3)) == NULL) _YYERROR("unknown_stmt"); @@ -1585,7 +1586,7 @@ unknown_stmt : ustring ':' ustring optsep ';' '{' yang_stmts '}' { if (ystack_pop(_yy) < 0) _YYERROR("unknown_stmt"); _PARSE_DEBUG("unknown-stmt -> ustring : ustring { yang-stmts }"); } - | ustring ':' ustring SEP string optsep + | ustring ':' ustring sep string optsep { char *id; if ((id=string_del_join($1, ":", $3)) == NULL) _YYERROR("unknown_stmt"); if (ysp_add_push(_yy, Y_UNKNOWN, id, $5) == NULL) _YYERROR("unknown_stmt"); } '{' yang_stmts '}' @@ -1777,6 +1778,8 @@ ustring : ustring CHARS } | CHARS { _PARSE_DEBUG1("ustring-> CHARS(%s)", $1); $$=$1; } + | ERRCHARS + { _PARSE_DEBUG1("ustring-> ERRCHARS(%s)", $1); _YYERROR("Invalid string chars"); } ; abs_schema_nodeid : abs_schema_nodeid '/' node_identifier @@ -1855,10 +1858,19 @@ node_identifier : IDENTIFIER identifier_ref : node_identifier { $$=$1;} ; -optsep : SEP +/* optsep = *(WSP / line-break) */ +optsep : sep | ; +/* sep = 1*(WSP / line-break) + * Note WS can in turn contain multiple white-space. + * Reason for doing list here is that the lex stage filters comments, + * For example, " // foo\n \t " will return WS WS + */ +sep : sep WS + | WS + ; stmtend : ';' | '{' '}'