From 72b133f5952f61eb8c695107cbb053d8a522494e Mon Sep 17 00:00:00 2001 From: Glenn Strauss Date: Thu, 23 Jun 2016 03:57:19 -0400 Subject: [PATCH] fix errors detected by Coverity Scan buffer.c:itostr() undefined behavior taking modulus of negative number additional minor code changes made to quiet other coverity warnings (false positives) --- src/buffer.c | 24 +++++++----------------- src/chunk.c | 4 ++-- src/http_auth.c | 2 ++ src/lempar.c | 13 +++++++++---- src/mod_accesslog.c | 24 +++++++++++++----------- src/mod_auth.c | 3 +++ src/mod_dirlisting.c | 4 ++-- src/mod_proxy.c | 2 +- src/network.c | 4 ++-- src/request.c | 13 ++++++------- 10 files changed, 47 insertions(+), 46 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index 97514357..b79ea992 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -270,23 +270,13 @@ static char* utostr(char * const buf_end, uintmax_t val) { } static char* itostr(char * const buf_end, intmax_t val) { - char *cur = buf_end; - if (val >= 0) return utostr(buf_end, (uintmax_t) val); - - /* can't take absolute value, as it isn't defined for INTMAX_MIN */ - do { - int mod = val % 10; - val /= 10; - /* val * 10 + mod == orig val, -10 < mod < 10 */ - /* we want a negative mod */ - if (mod > 0) { - mod -= 10; - val += 1; - } - /* prepend digit abs(mod) */ - *(--cur) = (char) ('0' + (-mod)); - } while (0 != val); - *(--cur) = '-'; + /* absolute value not defined for INTMAX_MIN, but can take absolute + * value of any negative number via twos complement cast to unsigned. + * negative sign is prepended after (now unsigned) value is converted + * to string */ + uintmax_t uval = val >= 0 ? (uintmax_t)val : ((uintmax_t)~val) + 1; + char *cur = utostr(buf_end, uval); + if (val < 0) *(--cur) = '-'; return cur; } diff --git a/src/chunk.c b/src/chunk.c index 7e7d1062..2bb57c3a 100644 --- a/src/chunk.c +++ b/src/chunk.c @@ -458,7 +458,7 @@ static chunk *chunkqueue_get_append_tempfile(chunkqueue *cq) { fd = mkstemp(template->ptr); } - if (-1 == fd) { + if (fd < 0) { buffer_free(template); return NULL; } @@ -498,7 +498,7 @@ int chunkqueue_append_mem_to_tempfile(server *srv, chunkqueue *dest, const char if (NULL != dst_c && FILE_CHUNK == dst_c->type && dst_c->file.is_temp - && -1 != dst_c->file.fd + && dst_c->file.fd >= 0 && 0 == dst_c->offset) { /* ok, take the last chunk for our job */ diff --git a/src/http_auth.c b/src/http_auth.c index 1b621830..8dfe9a6b 100644 --- a/src/http_auth.c +++ b/src/http_auth.c @@ -208,6 +208,7 @@ int http_auth_match_rules(server *srv, array *req, const char *username, const c UNUSED(host); require = (data_string *)array_get_element(req, "require"); + if (!require) return -1; /*(should not happen; config is validated at startup)*/ /* if we get here, the user we got a authed user */ if (0 == strcmp(require->value->ptr, "valid-user")) { @@ -721,6 +722,7 @@ int http_auth_basic_check(server *srv, connection *con, mod_auth_plugin_data *p, data_string *realm; realm = (data_string *)array_get_element(req, "realm"); + if (!realm) return 0; /*(should not happen; config is validated at startup)*/ username = buffer_init(); diff --git a/src/lempar.c b/src/lempar.c index 9c48fd20..309967c4 100644 --- a/src/lempar.c +++ b/src/lempar.c @@ -278,9 +278,10 @@ static void yy_destructor(YYCODETYPE yymajor, YYMINORTYPE *yypminor){ */ static int yy_pop_parser_stack(yyParser *pParser){ YYCODETYPE yymajor; - yyStackEntry *yytos = &pParser->yystack[pParser->yyidx]; + yyStackEntry *yytos; if( pParser->yyidx<0 ) return 0; + yytos = &pParser->yystack[pParser->yyidx]; #ifndef NDEBUG if( yyTraceFILE && pParser->yyidx>=0 ){ fprintf(yyTraceFILE,"%sPopping %s\n", @@ -460,10 +461,14 @@ static void yy_reduce( ParseARG_FETCH; yymsp = &yypParser->yystack[yypParser->yyidx]; #ifndef NDEBUG - if( yyTraceFILE && yyruleno>=0 + if( yyTraceFILE ) { + if (yyruleno>=0 && (size_t)yyrulenoconf.use_syslog) { /* syslog doesn't cache */ +#ifdef HAVE_SYSLOG_H + if (!buffer_string_is_empty(b)) { + /*(syslog appends a \n on its own)*/ + syslog(p->conf.syslog_level, "%s", b->ptr); + buffer_reset(b); + } +#endif + return HANDLER_GO_ON; + } + buffer_append_string_len(b, CONST_STR_LEN("\n")); - if (p->conf.use_syslog || /* syslog doesn't cache */ - (!buffer_string_is_empty(p->conf.access_logfile) && p->conf.access_logfile->ptr[0] == '|') || /* pipes don't cache */ + if ((!buffer_string_is_empty(p->conf.access_logfile) && p->conf.access_logfile->ptr[0] == '|') || /* pipes don't cache */ newts || buffer_string_length(b) >= BUFFER_MAX_REUSE_SIZE) { - if (p->conf.use_syslog) { -#ifdef HAVE_SYSLOG_H - if (!buffer_string_is_empty(b)) { - /* syslog appends a \n on its own */ - buffer_string_set_length(b, buffer_string_length(b) - 1); - syslog(p->conf.syslog_level, "%s", b->ptr); - } -#endif - } else if (p->conf.log_access_fd != -1) { + if (p->conf.log_access_fd >= 0) { accesslog_write_all(srv, p->conf.access_logfile, p->conf.log_access_fd, CONST_BUF_LEN(b)); } buffer_reset(b); diff --git a/src/mod_auth.c b/src/mod_auth.c index 9ec76dda..cfadba4b 100644 --- a/src/mod_auth.c +++ b/src/mod_auth.c @@ -295,6 +295,9 @@ static handler_t mod_auth_uri_handler(server *srv, connection *con, void *p_d) { con->http_status = 401; con->mode = DIRECT; + if (!method) return HANDLER_FINISHED;/*(should not happen; config is validated at startup)*/ + if (!realm) return HANDLER_FINISHED; /*(should not happen; config is validated at startup)*/ + if (0 == strcmp(method->value->ptr, "basic")) { buffer_copy_string_len(p->tmp_buf, CONST_STR_LEN("Basic realm=\"")); buffer_append_string_buffer(p->tmp_buf, realm->value); diff --git a/src/mod_dirlisting.c b/src/mod_dirlisting.c index 0ac95b2f..b65dbbac 100644 --- a/src/mod_dirlisting.c +++ b/src/mod_dirlisting.c @@ -684,9 +684,9 @@ static int http_list_directory(server *srv, connection *con, plugin_data *p, buf name_max = NAME_MAX; #endif - path = malloc(buffer_string_length(dir) + name_max + 1); + path = malloc(i + name_max + 1); force_assert(NULL != path); - strcpy(path, dir->ptr); + memcpy(path, dir->ptr, i+1); path_file = path + i; if (NULL == (dp = opendir(path))) { diff --git a/src/mod_proxy.c b/src/mod_proxy.c index 0ae53778..97fa1fc8 100644 --- a/src/mod_proxy.c +++ b/src/mod_proxy.c @@ -386,7 +386,7 @@ static int proxy_establish_connection(server *srv, handler_ctx *hctx) { memset(&proxy_addr_un, 0, sizeof(proxy_addr_un)); proxy_addr_un.sun_family = AF_UNIX; - strcpy(proxy_addr_un.sun_path, host->host->ptr); + memcpy(proxy_addr_un.sun_path, host->host->ptr, buffer_string_length(host->host) + 1); servlen = sizeof(proxy_addr_un); proxy_addr = (struct sockaddr *) &proxy_addr_un; } else diff --git a/src/network.c b/src/network.c index 91078eb4..a0b43835 100644 --- a/src/network.c +++ b/src/network.c @@ -1099,7 +1099,7 @@ int network_write_chunkqueue(server *srv, connection *con, chunkqueue *cq, off_t */ if (cq->first && cq->first->next) { corked = 1; - setsockopt(con->fd, IPPROTO_TCP, TCP_CORK, &corked, sizeof(corked)); + (void)setsockopt(con->fd, IPPROTO_TCP, TCP_CORK, &corked, sizeof(corked)); } #endif @@ -1119,7 +1119,7 @@ int network_write_chunkqueue(server *srv, connection *con, chunkqueue *cq, off_t #ifdef TCP_CORK if (corked) { corked = 0; - setsockopt(con->fd, IPPROTO_TCP, TCP_CORK, &corked, sizeof(corked)); + (void)setsockopt(con->fd, IPPROTO_TCP, TCP_CORK, &corked, sizeof(corked)); } #endif diff --git a/src/request.c b/src/request.c index 6432539c..50898602 100644 --- a/src/request.c +++ b/src/request.c @@ -16,7 +16,7 @@ static int request_check_hostname(buffer *host) { enum { DOMAINLABEL, TOPLABEL } stage = TOPLABEL; size_t i; int label_len = 0; - size_t host_len; + size_t host_len, hostport_len; char *colon; int is_ip = -1; /* -1 don't know yet, 0 no, 1 yes */ int level = 0; @@ -32,8 +32,6 @@ static int request_check_hostname(buffer *host) { * port = *digit */ - host_len = buffer_string_length(host); - /* IPv6 adress */ if (host->ptr[0] == '[') { char *c = host->ptr + 1; @@ -70,6 +68,8 @@ static int request_check_hostname(buffer *host) { return 0; } + hostport_len = host_len = buffer_string_length(host); + if (NULL != (colon = memchr(host->ptr, ':', host_len))) { char *c = colon + 1; @@ -88,12 +88,11 @@ static int request_check_hostname(buffer *host) { /* if the hostname ends in a "." strip it */ if (host->ptr[host_len-1] == '.') { /* shift port info one left */ - if (NULL != colon) memmove(colon-1, colon, buffer_string_length(host) - host_len); - buffer_string_set_length(host, buffer_string_length(host) - 1); - host_len -= 1; + if (NULL != colon) memmove(colon-1, colon, hostport_len - host_len); + buffer_string_set_length(host, --hostport_len); + if (--host_len == 0) return -1; } - if (host_len == 0) return -1; /* scan from the right and skip the \0 */ for (i = host_len; i-- > 0; ) { -- 2.11.4.GIT