Address various unchecked NULLs, deprecated interfaces, etc.

Aaron Drew · Aaron Drew · commit f7046234d187 · 2025-07-03T23:20:17.000+09:00
Tried building on MacOS for the first time so I've also made that work.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.7)
+cmake_minimum_required(VERSION 3.10)
 project(HttpsDnsProxy C)
 
 # FUNCTIONS
@@ -65,8 +65,21 @@ endif()
 
 # LIBRARY DEPENDENCIES
 
+# Add Homebrew paths for macOS
+if(APPLE)
+  if(EXISTS "/opt/homebrew")
+    list(APPEND CMAKE_PREFIX_PATH "/opt/homebrew")
+    link_directories("/opt/homebrew/lib")
+  elseif(EXISTS "/usr/local")
+    list(APPEND CMAKE_PREFIX_PATH "/usr/local")
+    link_directories("/usr/local/lib")
+  endif()
+endif()
+
 find_path(LIBCARES_INCLUDE_DIR ares.h)
 find_path(LIBEV_INCLUDE_DIR ev.h)
+find_library(LIBCARES_LIBRARY NAMES cares)
+find_library(LIBEV_LIBRARY NAMES ev)
 
 if(CUSTOM_LIBCURL_INSTALL_PATH)
   message(STATUS "Using custom libcurl from: ${CUSTOM_LIBCURL_INSTALL_PATH}")
@@ -108,7 +121,7 @@ set(TARGET_NAME "https_dns_proxy")
 aux_source_directory(src SRC_LIST)
 set(SRC_LIST ${SRC_LIST})
 add_executable(${TARGET_NAME} ${SRC_LIST})
-set(LIBS ${LIBS} cares curl ev resolv)
+set(LIBS ${LIBS} ${LIBCARES_LIBRARY} curl ${LIBEV_LIBRARY} resolv)
 target_link_libraries(${TARGET_NAME} ${LIBS})
 set_property(TARGET ${TARGET_NAME} PROPERTY C_STANDARD 11)
 
diff --git a/src/dns_poller.c b/src/dns_poller.c
@@ -44,21 +44,40 @@ static void sock_state_cb(void *data, int fd, int read, int write) {
   ev_io_start(d->loop, io_event_ptr);
 }
 
-static char *get_addr_listing(char** addr_list, const int af) {
+
+static char *get_addrinfo_listing(struct ares_addrinfo *ai) {
   char *list = (char *)calloc(1, POLLER_ADDR_LIST_SIZE);
   char *pos = list;
   if (list == NULL) {
     FLOG("Out of mem");
   }
-  for (int i = 0; addr_list[i]; i++) {
-    const char *res = ares_inet_ntop(af, addr_list[i], pos,
-                                     list + POLLER_ADDR_LIST_SIZE - 1 - pos);
+
+  for (struct ares_addrinfo_node *node = ai->nodes; node; node = node->ai_next) {
+    size_t remaining = POLLER_ADDR_LIST_SIZE - (pos - list) - 1;
+    if (remaining < INET6_ADDRSTRLEN + 1) { // +1 for comma
+      WLOG("Address list buffer too small, truncating");
+      break;
+    }
+
+    const char *res = NULL;
+    if (node->ai_family == AF_INET) {
+      struct sockaddr_in *sin = (struct sockaddr_in *)node->ai_addr;
+      res = ares_inet_ntop(AF_INET, &sin->sin_addr, pos, remaining);
+    } else if (node->ai_family == AF_INET6) {
+      struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)node->ai_addr;
+      res = ares_inet_ntop(AF_INET6, &sin6->sin6_addr, pos, remaining);
+    }
+
     if (res != NULL) {
       pos += strlen(pos);
+      if (pos - list >= POLLER_ADDR_LIST_SIZE - 1) {
+        break; // prevent overflow
+      }
       *pos = ',';
       pos++;
     }
   }
+
   if (pos == list) {
     free((void*)list);
     list = NULL;
@@ -68,19 +87,23 @@ static char *get_addr_listing(char** addr_list, const int af) {
   return list;
 }
 
-static void ares_cb(void *arg, int status, int __attribute__((unused)) timeouts,
-                    struct hostent *h) {
+static void ares_addrinfo_cb(void *arg, int status, int __attribute__((unused)) timeouts,
+                            struct ares_addrinfo *ai) {
   dns_poller_t *d = (dns_poller_t *)arg;
   d->request_ongoing = 0;
   ev_tstamp interval = 5;  // retry by default after some time
 
   if (status != ARES_SUCCESS) {
     WLOG("DNS lookup of '%s' failed: %s", d->hostname, ares_strerror(status));
-  } else if (!h || h->h_length < 1) {
+  } else if (!ai || !ai->nodes) {
     WLOG("No hosts found for '%s'", d->hostname);
   } else {
     interval = d->polling_interval;
-    d->cb(d->hostname, d->cb_data, get_addr_listing(h->h_addr_list, h->h_addrtype));
+    d->cb(d->hostname, d->cb_data, get_addrinfo_listing(ai));
+  }
+
+  if (ai) {
+    ares_freeaddrinfo(ai);
   }
 
   if (status != ARES_EDESTRUCTION) {
@@ -91,14 +114,15 @@ static void ares_cb(void *arg, int status, int __attribute__((unused)) timeouts,
   }
 }
 
+
 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 = tvp->tv_sec + tvp->tv_usec * 1e-6;
-    return after ? after : 0.1;
+    return after > 0 ? after : 0.1;
 }
 
 static void timer_cb(struct ev_loop __attribute__((unused)) *loop,
@@ -108,7 +132,7 @@ static void timer_cb(struct ev_loop __attribute__((unused)) *loop,
   if (d->request_ongoing) {
     // process query timeouts
     DLOG("Processing DNS queries");
-    ares_process(d->ares, NULL, NULL);
+    ares_process_fds(d->ares, NULL, 0, 0);
   } else {
     DLOG("Starting DNS query");
     // Cancel any pending queries before making new ones. c-ares can't be depended on to
@@ -117,7 +141,9 @@ static void timer_cb(struct ev_loop __attribute__((unused)) *loop,
     // free memory tied up by any "zombie" queries.
     ares_cancel(d->ares);
     d->request_ongoing = 1;
-    ares_gethostbyname(d->ares, d->hostname, d->family, ares_cb, d);
+    struct ares_addrinfo_hints hints = {0};
+    hints.ai_family = d->family;
+    ares_getaddrinfo(d->ares, d->hostname, NULL, &hints, ares_addrinfo_cb, d);
   }
 
   if (d->request_ongoing) {  // need to re-check, it might change!
@@ -134,6 +160,10 @@ void dns_poller_init(dns_poller_t *d, struct ev_loop *loop,
                      int bootstrap_dns_polling_interval,
                      const char *hostname,
                      int family, dns_poller_cb cb, void *cb_data) {
+  if (!d || !loop || !bootstrap_dns || !hostname || !cb) {
+    FLOG("Invalid parameters to dns_poller_init");
+  }
+
   int r = ares_library_init(ARES_LIB_INIT_ALL);
   if (r != ARES_SUCCESS) {
     FLOG("ares_library_init error: %s", ares_strerror(r));
diff --git a/src/dns_server.c b/src/dns_server.c
@@ -73,11 +73,13 @@ static void watcher_cb(struct ev_loop __attribute__((unused)) *loop,
                          &tmp_addrlen);
   if (len < 0) {
     ELOG("recvfrom failed: %s", strerror(errno));
+    free(buf);
     return;
   }
 
   if (len < (int)sizeof(uint16_t)) {
     WLOG("Malformed request received (too short).");
+    free(buf);
     return;
   }
 
diff --git a/src/https_client.c b/src/https_client.c
@@ -16,7 +16,9 @@
 
 #define DOH_CONTENT_TYPE "application/dns-message"
 enum {
-DOH_MAX_RESPONSE_SIZE = 65535
+DOH_MAX_RESPONSE_SIZE = 65535,
+HTTPS_TIMEOUT_WITH_CONNECTIONS = 5,
+HTTPS_TIMEOUT_WITHOUT_CONNECTIONS = 10
 };
 
 // the following macros require to have ctx pointer to https_fetch_ctx structure
@@ -310,10 +312,10 @@ static void https_fetch_ctx_init(https_client_t *client,
   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_USERAGENT, "https_dns_proxy/0.3");
+  ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_USERAGENT, "https_dns_proxy");
   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_TIMEOUT, client->connections > 0 ? HTTPS_TIMEOUT_WITH_CONNECTIONS : HTTPS_TIMEOUT_WITHOUT_CONNECTIONS);
   // 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_ERRORBUFFER, ctx->curl_errbuf); // zeroed by calloc
diff --git a/src/main.c b/src/main.c
@@ -36,13 +36,16 @@ typedef struct {
   struct sockaddr_storage raddr;
 } request_t;
 
-static int is_ipv4_address(char *str) {
+static int is_ipv4_address(const char *str) {
     struct in6_addr addr;
     return inet_pton(AF_INET, str, &addr) == 1;
 }
 
 static int hostname_from_url(const char* url_in,
                              char* hostname, const size_t hostname_len) {
+  if (!url_in || !hostname || hostname_len == 0) {
+    return 0;
+  }
   int res = 0;
   CURLU *url = curl_url();
   if (url != NULL) {
@@ -54,8 +57,9 @@ static int hostname_from_url(const char* url_in,
       if (rc == CURLUE_OK && host_len < hostname_len &&
           host[0] != '[' && host[host_len-1] != ']' && // skip IPv6 address
           !is_ipv4_address(host)) {
-        strncpy(hostname, host, hostname_len-1); // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
-        hostname[hostname_len-1] = '\0';
+        size_t copy_len = host_len < hostname_len - 1 ? host_len : hostname_len - 1;
+        memcpy(hostname, host, copy_len);
+        hostname[copy_len] = '\0';
         res = 1; // success
       }
       curl_free(host);
@@ -80,10 +84,11 @@ static void sigpipe_cb(struct ev_loop __attribute__((__unused__)) *loop,
 
 static void https_resp_cb(void *data, char *buf, size_t buflen) {
   request_t *req = (request_t *)data;
-  DLOG("Received response for id: %hX, len: %zu", req->tx_id, buflen);
   if (req == NULL) {
-    FLOG("%04hX: data NULL", req->tx_id);
+    FLOG("data NULL in https_resp_cb");
+    return;
   }
+  DLOG("Received response for id: %hX, len: %zu", req->tx_id, buflen);
   free((void*)req->dns_req);
   if (buf != NULL) { // May be NULL for timeout, DNS failure, or something similar.
     if (buflen < (int)sizeof(uint16_t)) {
@@ -107,6 +112,11 @@ static void https_resp_cb(void *data, char *buf, size_t buflen) {
 static void dns_server_cb(dns_server_t *dns_server, void *data,
                           struct sockaddr* addr, uint16_t tx_id,
                           char *dns_req, size_t dns_req_len) {
+  if (!dns_server || !data || !addr || !dns_req) {
+    ELOG("NULL pointer in dns_server_cb");
+    if (dns_req) free(dns_req);
+    return;
+  }
   app_state_t *app = (app_state_t *)data;
 
   DLOG("Received request for id: %hX, len: %d", tx_id, dns_req_len);
@@ -139,34 +149,63 @@ static void dns_server_cb(dns_server_t *dns_server, void *data,
 }
 
 static int addr_list_reduced(const char* full_list, const char* list) {
+  if (!full_list || !list) {
+    return 1;
+  }
+
   const char *pos = list;
-  const char *end = list + strlen(list);
+  const size_t list_len = strlen(list);
+  const char *end = list + list_len;
+
   while (pos < end) {
-    char current[50];
+    char current[INET6_ADDRSTRLEN];
     const char *comma = strchr(pos, ',');
-    size_t ip_len = comma ? comma - pos : end - pos;
-    strncpy(current, pos, ip_len); // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
+    size_t ip_len = comma ? (size_t)(comma - pos) : (size_t)(end - pos);
+
+    if (ip_len == 0 || ip_len >= sizeof(current)) {
+      DLOG("Invalid IP address length: %zu", ip_len);
+      return 1;
+    }
+
+    memcpy(current, pos, ip_len);
     current[ip_len] = '\0';
 
-    const char *match_begin = strstr(full_list, current);
-    if (!match_begin ||
-        !(match_begin == full_list || *(match_begin - 1) == ',') ||
-        !(*(match_begin + ip_len) == ',' || *(match_begin + ip_len) == '\0')) {
+    // More efficient search: check if the current IP exists as a complete token
+    const char *search_pos = full_list;
+    int found = 0;
+
+    while ((search_pos = strstr(search_pos, current)) != NULL) {
+      // Check if it's a complete token (preceded and followed by delimiter or boundary)
+      int is_start = (search_pos == full_list || *(search_pos - 1) == ',');
+      int is_end = (*(search_pos + ip_len) == ',' || *(search_pos + ip_len) == '\0');
+
+      if (is_start && is_end) {
+        found = 1;
+        break;
+      }
+      search_pos += ip_len;
+    }
+
+    if (!found) {
       DLOG("IP address missing: %s", current);
       return 1;
     }
 
-    pos += ip_len + 1;
+    pos = comma ? comma + 1 : end;
   }
   return 0;
 }
 
 static void dns_poll_cb(const char* hostname, void *data,
                         const char* addr_list) {
+  if (!hostname || !data || !addr_list) {
+    ELOG("NULL pointer in dns_poll_cb");
+    return;
+  }
   app_state_t *app = (app_state_t *)data;
   char buf[255 + (sizeof(":443:") - 1) + POLLER_ADDR_LIST_SIZE];
   memset(buf, 0, sizeof(buf)); // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
-  if (strlen(hostname) > 254) { FLOG("Hostname too long."); }
+  if (strlen(hostname) > 253) { FLOG("Hostname too long."); }
   int ip_start = snprintf(buf, sizeof(buf) - 1, "%s:443:", hostname);  // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   (void)snprintf(buf + ip_start, sizeof(buf) - 1 - ip_start, "%s", addr_list); // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
   if (app->resolv && app->resolv->data) {
@@ -205,7 +244,7 @@ static int proxy_supports_name_resolution(const char *proxy)
   return 0;
 }
 
-static const char * sw_version(void) {
+static const char *sw_version(void) {
 #ifdef SW_VERSION
   return SW_VERSION;
 #else
@@ -315,10 +354,27 @@ int main(int argc, char *argv[]) {
   }
 
   if (opt.daemonize) {
-    // daemon() is non-standard. If needed, see OpenSSH openbsd-compat/daemon.c
-    if (daemon(0, 0) == -1) {
-      FLOG("daemon failed: %s", strerror(errno));
+    // Use fork() and setsid() instead of deprecated daemon()
+    pid_t pid = fork();
+    if (pid < 0) {
+      FLOG("fork failed: %s", strerror(errno));
+    } else if (pid > 0) {
+      exit(0); // parent exits
+    }
+
+    if (setsid() < 0) {
+      FLOG("setsid failed: %s", strerror(errno));
     }
+
+    // Change working directory to root
+    if (chdir("/") < 0) {
+      FLOG("chdir failed: %s", strerror(errno));
+    }
+
+    // Close standard file descriptors
+    close(STDIN_FILENO);
+    close(STDOUT_FILENO);
+    close(STDERR_FILENO);
   }
 
   ev_signal sigpipe;
diff --git a/src/options.c b/src/options.c
@@ -123,16 +123,24 @@ enum OptionsParseResult options_parse_args(struct Options *opt, int argc, char *
 
   if (opt->user) {
     struct passwd *p = getpwnam(opt->user);
-    if (!p || !p->pw_uid) {
-      printf("Username (%s) invalid.\n", opt->user);
+    if (!p) {
+      printf("Username (%s) not found.\n", opt->user);
+      return OPR_OPTION_ERROR;
+    }
+    if (p->pw_uid == 0) {
+      printf("Username (%s) is root (uid 0), which is not allowed.\n", opt->user);
       return OPR_OPTION_ERROR;
     }
     opt->uid = p->pw_uid;
   }
   if (opt->group) {
     struct group *g = getgrnam(opt->group);
-    if (!g || !g->gr_gid) {
-      printf("Group (%s) invalid.\n", opt->group);
+    if (!g) {
+      printf("Group (%s) not found.\n", opt->group);
+      return OPR_OPTION_ERROR;
+    }
+    if (g->gr_gid == 0) {
+      printf("Group (%s) is root (gid 0), which is not allowed.\n", opt->group);
       return OPR_OPTION_ERROR;
     }
     opt->gid = g->gr_gid;
diff --git a/src/ring_buffer.c b/src/ring_buffer.c
diff --git a/tests/robot/functional_tests.robot b/tests/robot/functional_tests.robot

Original file line number	Diff line number	Diff line change
`@@ -73,11 +73,13 @@ static void watcher_cb(struct ev_loop __attribute__((unused)) *loop,`
`73`	`73`	`&tmp_addrlen);`
`74`	`74`	`if (len < 0) {`
`75`	`75`	`ELOG("recvfrom failed: %s", strerror(errno));`
	`76`	`+ free(buf);`
`76`	`77`	`return;`
`77`	`78`	`}`
`78`	`79`
`79`	`80`	`if (len < (int)sizeof(uint16_t)) {`
`80`	`81`	`WLOG("Malformed request received (too short).");`
	`82`	`+ free(buf);`
`81`	`83`	`return;`
`82`	`84`	`}`
`83`	`85`