Fixes 16 (#193)

* Warning fixes

Compatibility with CMake < 3.10 will be removed
from a future version of CMake.

warning: call to '_curl_easy_setopt_err_long' declared with
attribute warning: curl_easy_setopt expects a long argument

* Avoid division by zero

The operation of dividing by a fraction is
equivalent to multiplying by its reciprocal.

* clang tidy changes

* Replace atoi with strtoul

Safer, minus one clang tidy rule.

Author: baranyaib90

CMakeLists.txt

@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.7)
+cmake_minimum_required(VERSION 3.10)
 project(HttpsDnsProxy C)
 
 include(CheckIncludeFile)
@@ -110,7 +110,7 @@ if(USE_CLANG_TIDY)
     message(STATUS "clang-tidy not found.")
   else()
     message(STATUS "clang-tidy found: ${CLANG_TIDY_EXE}")
-    set(DO_CLANG_TIDY "${CLANG_TIDY_EXE}" "-fix" "-fix-errors" "-checks=*,-cert-err34-c,-readability-identifier-length,-altera-unroll-loops,-bugprone-easily-swappable-parameters,-concurrency-mt-unsafe,-*magic-numbers,-hicpp-signed-bitwise,-readability-function-cognitive-complexity,-altera-id-dependent-backward-branch,-google-readability-todo,-misc-include-cleaner,-cast-align")
+    set(DO_CLANG_TIDY "${CLANG_TIDY_EXE}" "-fix" "-fix-errors" "-checks=*,-readability-identifier-length,-altera-unroll-loops,-bugprone-easily-swappable-parameters,-concurrency-mt-unsafe,-*magic-numbers,-hicpp-signed-bitwise,-readability-function-cognitive-complexity,-altera-id-dependent-backward-branch,-misc-include-cleaner,-llvmlibc-restrict-system-libc-headers,-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
   endif()
 else()
   message(STATUS "Not using clang-tidy.")

src/dns_poller.c

@@ -1,5 +1,5 @@
-#include <netdb.h>       // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <string.h>      // NOLINT(llvmlibc-restrict-system-libc-headers)
+#include <netdb.h>
+#include <string.h>
 
 #include "dns_poller.h"
 #include "logging.h"
@@ -38,7 +38,6 @@ static void sock_state_cb(void *data, int fd, int read, int write) {
     FLOG("c-ares needed more IO event handler, than the number of provided nameservers: %u", d->io_events_count);
   }
   DLOG("Reserved new io event: %p", io_event_ptr);
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   ev_io_init(io_event_ptr, sock_cb, fd,
              (read ? EV_READ : 0) | (write ? EV_WRITE : 0));
   ev_io_start(d->loop, io_event_ptr);
@@ -119,7 +118,6 @@ static ev_tstamp get_timeout(dns_poller_t *d)
     static struct timeval max_tv = {.tv_sec = 5, .tv_usec = 0};
     struct timeval tv;
     struct timeval *tvp = ares_timeout(d->ares, &max_tv, &tv);
-    // NOLINTNEXTLINE(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
     ev_tstamp after = (double)tvp->tv_sec + (double)tvp->tv_usec * 1e-6;
     return after > 0.1 ? after : 0.1;
 }
@@ -147,7 +145,7 @@ static void timer_cb(struct ev_loop __attribute__((unused)) *loop,
     d->request_ongoing = 1;
 
     struct ares_addrinfo_hints hints;
-    memset(&hints, 0, sizeof(hints));  // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
+    memset(&hints, 0, sizeof(hints));
     hints.ai_flags  = ARES_AI_CANONNAME;
     hints.ai_family = d->family;
     hints.ai_socktype = SOCK_STREAM;
@@ -199,8 +197,6 @@ void dns_poller_init(dns_poller_t *d, struct ev_loop *loop,
   d->polling_interval = bootstrap_dns_polling_interval;
   d->request_ongoing = 0;
   d->cb_data = cb_data;
-
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   ev_timer_init(&d->timer, timer_cb, 0, 0);
   d->timer.data = d;
   ev_timer_start(d->loop, &d->timer);

src/dns_server.c

@@ -1,9 +1,9 @@
-#include <ares.h>            // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <ares_dns_record.h> // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <errno.h>           // NOLINT(llvmlibc-restrict-system-libc-headers)
+#include <ares.h>
+#include <ares_dns_record.h>
+#include <errno.h>
 #include <stdint.h>
-#include <string.h>          // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <unistd.h>          // NOLINT(llvmlibc-restrict-system-libc-headers)
+#include <string.h>
+#include <unistd.h>
 
 #include "dns_server.h"
 #include "logging.h"
@@ -67,7 +67,7 @@ static void watcher_cb(struct ev_loop __attribute__((unused)) *loop,
   if (dns_req == NULL) {
     FLOG("Out of mem");
   }
-  memcpy(dns_req, tmp_buf, (size_t)len);  // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
+  memcpy(dns_req, tmp_buf, (size_t)len);
 
   d->cb(d, 0, d->cb_data, (struct sockaddr*)&tmp_raddr, dns_req, (size_t)len);
 }
@@ -80,8 +80,6 @@ void dns_server_init(dns_server_t *d, struct ev_loop *loop,
   d->addrlen = listen_addrinfo->ai_addrlen;
   d->cb = cb;
   d->cb_data = data;
-
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   ev_io_init(&d->watcher, watcher_cb, d->sock, EV_READ);
   d->watcher.data = d;
   ev_io_start(d->loop, &d->watcher);
@@ -145,7 +143,7 @@ static void truncate_dns_response(char *buf, size_t *buflen, const uint16_t size
 
   // rough estimate to reach size limit
   size_t answers = ares_dns_record_rr_cnt(dnsrec, ARES_SECTION_ANSWER);
-  size_t answers_to_keep = (size_limit - DNS_HEADER_LENGTH) / (old_size / answers);
+  size_t answers_to_keep = ((size_limit - DNS_HEADER_LENGTH) * answers) / old_size;
   answers_to_keep = answers_to_keep > 0 ? answers_to_keep : 1;  // try to keep 1 answer
 
   // remove answer records until fit size limit or running out of answers
@@ -185,7 +183,7 @@ static void truncate_dns_response(char *buf, size_t *buflen, const uint16_t size
   ares_dns_record_destroy(dnsrec);
 
   if (new_resp != NULL && new_resp_len < old_size) {
-    memcpy(buf, new_resp, new_resp_len);  // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
+    memcpy(buf, new_resp, new_resp_len);
     *buflen = new_resp_len;
     buf[2] |= 0x02;  // set truncation flag
     ILOG("%04hX: DNS response size truncated from %u to %u to keep %u limit",
@@ -196,6 +194,10 @@ static void truncate_dns_response(char *buf, size_t *buflen, const uint16_t size
 
 void dns_server_respond(dns_server_t *d, struct sockaddr *raddr,
     const char *dns_req, const size_t dns_req_len, char *dns_resp, size_t dns_resp_len) {
+  if (dns_resp_len < DNS_HEADER_LENGTH) {
+    WLOG("Malformed response received, invalid length: %u", dns_resp_len);
+    return;
+  }
   if (dns_resp_len > DNS_SIZE_LIMIT) {
     const uint16_t udp_size = get_edns_udp_size(dns_req, dns_req_len);
     if (dns_resp_len > udp_size) {

src/dns_server_tcp.c

@@ -1,9 +1,9 @@
 //NOLINTNEXTLINE(bugprone-reserved-identifier,cert-dcl37-c,cert-dcl51-cpp)
 #define _GNU_SOURCE  // needed for having accept4()
 
-#include <errno.h>   // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <fcntl.h>   // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <unistd.h>  // NOLINT(llvmlibc-restrict-system-libc-headers)
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
 
 #include "dns_server_tcp.h"
 #include "logging.h"
@@ -104,10 +104,10 @@ static int get_dns_request(struct tcp_client_s *client,
   if (*dns_req == NULL) {
     FLOG_CLIENT("Out of mem");
   }
-  memcpy(*dns_req, client->input_buffer + sizeof(uint16_t), *req_size);  // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
+  memcpy(*dns_req, client->input_buffer + sizeof(uint16_t), *req_size);
   // move down data of next request(s) if any
   client->input_buffer_used -= data_size;
-  memmove(client->input_buffer, client->input_buffer + data_size, client->input_buffer_used);  // NOLINT(clang-diagnostic-format-nonliteral,clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
+  memmove(client->input_buffer, client->input_buffer + data_size, client->input_buffer_used);
   return 1;
 }
 
@@ -150,7 +150,7 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop,
       FLOG_CLIENT("Out of mem");
     }
   }
-  memcpy(client->input_buffer + client->input_buffer_used, buf, (size_t)len);  // NOLINT(clang-diagnostic-format-nonliteral,clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
+  memcpy(client->input_buffer + client->input_buffer_used, buf, (size_t)len);
   client->input_buffer_used = needed_space;
 
   // Split requests
@@ -208,13 +208,12 @@ static void accept_cb(struct ev_loop __attribute__((unused)) *loop,
   client->d = d;
   client->id = d->client_id;
   client->sock = client_sock;
-  memcpy(&client->raddr, &client_addr, client_addr_len);  // NOLINT(clang-diagnostic-format-nonliteral,clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
+  memcpy(&client->raddr, &client_addr, client_addr_len);
   client->addr_len = client_addr_len;
   client->input_buffer = NULL;
   client->next = d->clients;
   d->clients = client;
 
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   ev_io_init(&client->read_watcher, read_cb, client->sock, EV_READ);
   client->read_watcher.data = client;
   ev_io_start(d->loop, &client->read_watcher);
@@ -294,7 +293,6 @@ dns_server_tcp_t * dns_server_tcp_create(
   d->client_limit = tcp_client_limit;
   d->clients = NULL;
 
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   ev_io_init(&d->accept_watcher, accept_cb, d->sock, EV_READ);
   d->accept_watcher.data = d;
   ev_io_start(d->loop, &d->accept_watcher);

src/https_client.c

@@ -1,13 +1,13 @@
-#include <ctype.h>         // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <errno.h>         // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <ev.h>            // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <math.h>          // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <netinet/in.h>    // NOLINT(llvmlibc-restrict-system-libc-headers)
+#include <ctype.h>
+#include <errno.h>
+#include <ev.h>
+#include <math.h>
+#include <netinet/in.h>
 #include <stdint.h>
-#include <stdio.h>         // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <string.h>        // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <sys/socket.h>    // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <unistd.h>        // NOLINT(llvmlibc-restrict-system-libc-headers)
+#include <stdio.h>
+#include <string.h>
+#include <sys/socket.h>
+#include <unistd.h>
 
 #include "https_client.h"
 #include "logging.h"
@@ -69,7 +69,6 @@ static size_t write_buffer(void *buf, size_t size, size_t nmemb, void *userp) {
     return 0;
   }
   ctx->buf = new_buf;
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   memcpy(&(ctx->buf[ctx->buflen]), buf, write_size);
   ctx->buflen = new_size;
   // We always expect to receive valid non-null ASCII but just to be safe...
@@ -153,21 +152,16 @@ static void https_log_data(int level, struct https_fetch_ctx *ctx,
     char str[width + 1];
     size_t hex_off = 0;
     size_t str_off = 0;
-    // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
     memset(hex, 0, sizeof(hex));
-    // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
     memset(str, 0, sizeof(str));
 
     for (size_t c = 0; c < width; c++) {
       if (i+c < size) {
-        // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
         hex_off += (size_t)snprintf(hex + hex_off, sizeof(hex) - hex_off,
                                     "%02x ", (unsigned char)ptr[i+c]);
-        // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
         str_off += (size_t)snprintf(str + str_off, sizeof(str) - str_off,
                                     "%c", isprint(ptr[i+c]) ? ptr[i+c] : '.');
       } else {
-        // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
         hex_off += (size_t)snprintf(hex + hex_off, sizeof(hex) - hex_off, "   ");
       }
     }
@@ -313,14 +307,14 @@ static void https_fetch_ctx_init(https_client_t *client,
   ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_POSTFIELDS, data);
   ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_WRITEFUNCTION, &write_buffer);
   ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_WRITEDATA, ctx);
-  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_MAXAGE_CONN, client->opt->max_idle_time);
-  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_PIPEWAIT, client->opt->use_http_version > 1);
+  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_MAXAGE_CONN, (long)client->opt->max_idle_time);
+  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_PIPEWAIT, (long)(client->opt->use_http_version > 1));
   ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_USERAGENT, "https_dns_proxy/0.4");
-  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_FOLLOWLOCATION, 0);
-  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_NOSIGNAL, 0);
-  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_TIMEOUT, client->connections > 0 ? 5 : 10 /* seconds */);
+  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_FOLLOWLOCATION, 0L);
+  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_NOSIGNAL, 0L);
+  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_TIMEOUT, client->connections > 0 ? 5L : 10L /* seconds */);
   // We know Google supports this, so force it.
-  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2);
+  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_SSLVERSION, (long)CURL_SSLVERSION_TLSv1_2);
   ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_ERRORBUFFER, ctx->curl_errbuf); // zeroed by calloc
   if (client->opt->curl_proxy) {
     DLOG_REQ("Using curl proxy: %s", client->opt->curl_proxy);
@@ -639,7 +633,6 @@ static int multi_sock_cb(CURL *curl, curl_socket_t sock, int what,
     return -1;
   }
   DLOG("Reserved new io event: %p", io_event_ptr);
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   ev_io_init(io_event_ptr, sock_cb, sock,
              ((what & CURL_POLL_IN) ? EV_READ : 0) |
              ((what & CURL_POLL_OUT) ? EV_WRITE : 0));
@@ -652,7 +645,6 @@ static int multi_timer_cb(CURLM __attribute__((unused)) *multi,
   GET_PTR(https_client_t, c, userp);
   ev_timer_stop(c->loop, &c->timer);
   if (timeout_ms >= 0) {
-    // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
     ev_timer_init(&c->timer, timer_cb, (double)timeout_ms / 1000.0, 0);
     ev_timer_start(c->loop, &c->timer);
   }
@@ -681,7 +673,6 @@ static void reset_timer_cb(struct ev_loop __attribute__((unused)) *loop,
 
 void https_client_init(https_client_t *c, options_t *opt,
                        stat_t *stat, struct ev_loop *loop) {
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   memset(c, 0, sizeof(*c));
   c->loop = loop;
   c->fetches = NULL;

src/logging.c

@@ -1,9 +1,9 @@
 #include <inttypes.h>
 #include <stdarg.h>
 #include <stdint.h>
-#include <stdio.h>         // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <sys/time.h>      // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <unistd.h>        // NOLINT(llvmlibc-restrict-system-libc-headers)
+#include <stdio.h>
+#include <sys/time.h>
+#include <unistd.h>
 
 #include "logging.h"
 #include "ring_buffer.h"
@@ -56,7 +56,6 @@ void logging_events_init(struct ev_loop *loop) {
   /* don't start timer if we will never write messages that are not flushed */
   if (loglevel < LOG_FLUSH_LEVEL) {
     DLOG("starting periodic log flush timer");
-    // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
     ev_timer_init(&logging_timer, logging_timer_cb, 0, 10);
     ev_timer_start(loop, &logging_timer);
   }
@@ -114,8 +113,6 @@ void _log(const char *file, int line, int severity, const char *fmt, ...) {
 
   char buff[LOG_LINE_SIZE];
   uint32_t buff_pos = 0;
-
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   int chars = snprintf(buff, LOG_LINE_SIZE, "%s %8"PRIu64".%06"PRIu64" %s:%d ",
                        SeverityStr[severity], (uint64_t)tv.tv_sec, (uint64_t)tv.tv_usec, file, line);
   if (chars < 0 || chars >= LOG_LINE_SIZE/2) {
@@ -125,8 +122,7 @@ void _log(const char *file, int line, int severity, const char *fmt, ...) {
 
   va_list args;
   va_start(args, fmt);
-  // NOLINTNEXTLINE(clang-diagnostic-format-nonliteral,clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
-  chars = vsnprintf(buff + buff_pos, LOG_LINE_SIZE - buff_pos, fmt, args);
+  chars = vsnprintf(buff + buff_pos, LOG_LINE_SIZE - buff_pos, fmt, args);  // NOLINT(clang-diagnostic-format-nonliteral)
   va_end(args);
 
   if (chars < 0) {
@@ -145,8 +141,6 @@ void _log(const char *file, int line, int severity, const char *fmt, ...) {
   if (severity < loglevel) {
     return;
   }
-
-  // NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   (void)fprintf(logfile, "%s\n", buff);
 
   if (severity >= LOG_FLUSH_LEVEL) {

src/logging.h

@@ -1,9 +1,9 @@
 #ifndef _LOGGING_H_
 #define _LOGGING_H_
 
-#include <stdio.h>         // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <stdlib.h>        // NOLINT(llvmlibc-restrict-system-libc-headers)
-#include <ev.h>            // NOLINT(llvmlibc-restrict-system-libc-headers)
+#include <stdio.h>
+#include <stdlib.h>
+#include <ev.h>
 
 #ifdef __cplusplus
 extern "C" {