Fix dangling hostname pointer in dns_poller; review cleanups

CMakeLists.txt

@@ -131,13 +131,20 @@ if(USE_CLANG_TIDY)
     message(STATUS "clang-tidy not found.")
   else()
     message(STATUS "clang-tidy found: ${CLANG_TIDY_EXE}")
-    set(CLANG_TIDY_CHECKS "-checks=*,-readability-identifier-length,-altera-unroll-loops,-concurrency-mt-unsafe,")
-    string(APPEND CLANG_TIDY_CHECKS "-bugprone-easily-swappable-parameters,-*magic-numbers,-hicpp-signed-bitwise,")
-    string(APPEND CLANG_TIDY_CHECKS "-readability-function-cognitive-complexity,-altera-id-dependent-backward-branch,")
-    string(APPEND CLANG_TIDY_CHECKS "-misc-include-cleaner,-llvmlibc-restrict-system-libc-headers,")
+    # Opt in to the check modules relevant to portable C instead of "*": the
+    # wildcard also enables domain-specific modules (altera-* for FPGA/OpenCL
+    # kernels, llvmlibc-*, android-*, ...) whose fixits have previously written
+    # inappropriate attributes into this codebase via -fix (see altera-struct-
+    # pack-align: extended alignment is UB on calloc'd structs, and packing
+    # misaligns embedded sockaddr_storage/libev watchers).
+    set(CLANG_TIDY_CHECKS "-checks=-*,bugprone-*,cert-*,clang-analyzer-*,concurrency-*,")
+    string(APPEND CLANG_TIDY_CHECKS "misc-*,performance-*,portability-*,readability-*,")
+    string(APPEND CLANG_TIDY_CHECKS "-readability-identifier-length,-concurrency-mt-unsafe,")
+    string(APPEND CLANG_TIDY_CHECKS "-bugprone-easily-swappable-parameters,-*magic-numbers,")
+    string(APPEND CLANG_TIDY_CHECKS "-readability-function-cognitive-complexity,-misc-include-cleaner,")
     string(APPEND CLANG_TIDY_CHECKS "-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,")
     string(APPEND CLANG_TIDY_CHECKS "-*function-size,-clang-diagnostic-*variadic-macro-arguments*")
-    set(DO_CLANG_TIDY "${CLANG_TIDY_EXE}" "-fix" "-fix-errors" "${CLANG_TIDY_CHECKS}")
+    set(DO_CLANG_TIDY "${CLANG_TIDY_EXE}" "${CLANG_TIDY_CHECKS}")
   endif()
 else()
   message(STATUS "Not using clang-tidy.")

src/dns_listener_tcp.c

@@ -55,7 +55,7 @@ struct tcp_client_s {
   ev_timer timer_watcher;
 
   struct tcp_client_s * next;
-} __attribute__((packed)) __attribute__((aligned(128)));
+};
 
 struct dns_listener_tcp_s {
   dns_listener_t base;
@@ -73,7 +73,7 @@ struct dns_listener_tcp_s {
   uint16_t client_count;
   uint16_t client_limit;
   struct tcp_client_s * clients;
-} __attribute__((packed)) __attribute__((aligned(128)));
+};
 
 
 static void remove_client(struct tcp_client_s * client) {
@@ -332,7 +332,7 @@ static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr,
   // Limit response size to prevent overflow when accounting for the 2-byte
   // length prefix. The total on-wire size would be resp_len + sizeof(uint16_t).
   if (resp_len < DNS_HEADER_LENGTH || resp_len > TCP_DNS_MAX_PAYLOAD) {
-    WLOG("Malformed response received, invalid length: %u", resp_len);
+    WLOG("Malformed response received, invalid length: %zu", resp_len);
     return;
   }
   const uint16_t response_id = ntohs(*((uint16_t*)resp));
@@ -355,7 +355,7 @@ static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr,
   // below is a blocking syscall (not an event loop yield). If remove_client()
   // is called due to send errors, the function returns immediately.
 
-  DLOG_CLIENT("Sending %u bytes", resp_len);
+  DLOG_CLIENT("Sending %zu bytes", resp_len);
 
   // send length of response
   uint16_t resp_size = htons((uint16_t)resp_len);

src/dns_listener_udp.c

@@ -21,7 +21,7 @@ typedef struct dns_listener_udp_s {
 
   dns_request_fn cb;
   void *cb_data;
-} __attribute__((aligned(128))) dns_listener_udp_t;
+} dns_listener_udp_t;
 
 // Creates and binds a listening UDP socket for incoming requests.
 static int get_listen_sock(struct addrinfo *listen_addrinfo) {
@@ -91,7 +91,7 @@ static void udp_respond(dns_listener_t *self, struct sockaddr *raddr,
   dns_listener_udp_t *d = (dns_listener_udp_t *)self;
 
   if (dns_resp_len < DNS_HEADER_LENGTH) {
-    WLOG("Malformed response received, invalid length: %u", dns_resp_len);
+    WLOG("Malformed response received, invalid length: %zu", dns_resp_len);
     return;
   }
   dns_truncate_for_udp(dns_req, dns_req_len, dns_resp, &dns_resp_len);

src/dns_poller.c

@@ -237,7 +237,10 @@ void dns_poller_init(dns_poller_t *d, struct ev_loop *loop,
   }
 
   d->loop = loop;
-  d->hostname = hostname;
+  d->hostname = strdup(hostname);
+  if (d->hostname == NULL) {
+    FLOG("Out of mem");
+  }
   d->family = family;
   set_bootstrap_source_addr(d->ares, source_addr, family);
   d->cb = cb;
@@ -269,5 +272,6 @@ void dns_poller_cleanup(dns_poller_t *d) {
   ares_destroy(d->ares);
   ev_timer_stop(d->loop, &d->timer);
   ares_library_cleanup();
+  free(d->hostname);
   free(d->io_events);
 }

src/dns_poller.h

@@ -21,7 +21,7 @@ typedef void (*dns_poller_cb)(const char* hostname, void *data,
 typedef struct {
   ares_channel ares;
   struct ev_loop *loop;
-  const char *hostname;
+  char *hostname;  // owned; strdup'd in init, freed in cleanup
   int family;  // AF_UNSPEC for IPv4 or IPv6, AF_INET for IPv4 only.
   dns_poller_cb cb;
   int polling_interval;
@@ -40,8 +40,7 @@ typedef struct {
 // `source_addr` optionally binds bootstrap DNS lookups to a specific IP.
 // `family` should be AF_INET for IPv4 or AF_UNSPEC for both IPv4 and IPv6.
 //
-// Note: hostname *not* copied. It should remain valid until
-// dns_poller_cleanup called.
+// Note: hostname is copied; the caller's buffer need not outlive this call.
 void dns_poller_init(dns_poller_t *d, struct ev_loop *loop,
                      const char *bootstrap_dns,
                      int bootstrap_dns_polling_interval,

src/dns_truncate.c

@@ -75,9 +75,9 @@ static uint16_t get_edns_udp_size(const char *dns_req, const size_t dns_req_len)
  * 3. EDNS0/OPT Preservation (RFC 6891):
  * The OPT pseudo-RR (Type 41) is critical for extended error tracking, cookies, and DNSSEC signaling.
  * RFC 6891 mandates that OPT records should be preserved in truncated messages if they were present
- * in the request. This function scans the Additional section, locates the OPT record, and uses
- * memmove() to safely relocate it to sit directly flush against the end of the Question section,
- * preserving it in the truncated response stream.
+ * in the request. This function scans the Additional section and removes every record except the
+ * OPT one; re-serializing via c-ares then leaves the OPT record directly after the Question
+ * section, preserving it in the truncated response stream.
  *
  * 4. Trusted Data Assumption:
  * DoH resolver response is considered trusted input, so assuming that it complies with RFCs
@@ -142,7 +142,7 @@ static void truncate_to_size_limit(uint8_t *buf, size_t *buflen, size_t size_lim
     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",
+    ILOG("%04hX: DNS response size truncated from %zu to %zu to keep %zu limit",
          resp_id, old_size, new_resp_len, size_limit);
   }
 

src/doh_proxy.c

@@ -25,7 +25,7 @@ struct doh_proxy {
   // fallback resolver and risk a recursion through our own listener).
   uint8_t awaiting_bootstrap;
   doh_proxy_bootstrap_done_cb bootstrap_done_cb;
-} __attribute__((aligned(64)));
+};
 
 // Per-request transient state. Lives from doh_proxy_handle_request to
 // https_resp_cb, when the response (or failure) returns from libcurl.
@@ -36,7 +36,7 @@ typedef struct {
   struct sockaddr_storage raddr;
   char *dns_req;
   size_t dns_req_len;
-} __attribute__((packed)) __attribute__((aligned(128))) doh_request_t;
+} doh_request_t;
 
 doh_proxy_t * doh_proxy_create(struct ev_loop *loop,
                                https_client_t *client,
@@ -146,7 +146,9 @@ void doh_proxy_handle_resolver_update(const char *hostname, void *ctx,
 
   if (p->awaiting_bootstrap) {
     p->awaiting_bootstrap = 0;
-    p->bootstrap_done_cb();
+    if (p->bootstrap_done_cb != NULL) {
+      p->bootstrap_done_cb();
+    }
   }
 }
 
@@ -162,7 +164,7 @@ static void doh_response_cb(void *data, char *buf, size_t buflen) {
 
   if (buf != NULL) {  // NULL on timeout / DNS failure / similar.
     if (buflen < DNS_HEADER_LENGTH) {
-      WLOG("%04hX: Malformed response received, too short: %u", req_id, buflen);
+      WLOG("%04hX: Malformed response received, too short: %zu", req_id, buflen);
     } else {
       const uint16_t resp_id = ntohs(*((uint16_t*)buf));
       if (req_id != resp_id) {

src/main.c

@@ -24,11 +24,16 @@
 #include "options.h"
 #include "stat.h"
 
-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 is_ip_address(const char *str) {
+    struct in6_addr addr;
+    return is_ipv4_address(str) || inet_pton(AF_INET6, str, &addr) == 1;
+}
+
 enum url_type {
   URL_TYPE_ERROR,
   URL_TYPE_IP,
@@ -305,13 +310,19 @@ int main(int argc, char *argv[]) {
         doh_proxy_set_port(proxy, port);
         if (opt.resolver_ip == NULL) {
           dns_poller = (dns_poller_t *)calloc(1, sizeof(dns_poller_t));
+          if (dns_poller == NULL) {
+            FLOG("Out of mem");
+          }
           doh_proxy_await_bootstrap(proxy, systemd_notify_ready);
           dns_poller_init(dns_poller, loop, opt.bootstrap_dns,
                           opt.bootstrap_dns_polling_interval, opt.source_addr,
                           hostname, opt.ipv4 ? AF_INET : AF_UNSPEC,
                           doh_proxy_handle_resolver_update, proxy);
           ILOG("DNS polling initialized for '%s'", hostname);
         } else {
+          if (!is_ip_address(opt.resolver_ip)) {
+            FLOG("Resolver IP override '%s' is not a valid IP literal", opt.resolver_ip);
+          }
           const size_t resolv_buf_len = strlen(hostname) + 1 + PORT_STR_LENGTH + 1 + strlen(opt.resolver_ip) + 1;
           char * resolv_buf = (char *)calloc(resolv_buf_len, sizeof(char));
           (void)snprintf(resolv_buf, resolv_buf_len, "%s:%u:%s", hostname, port, opt.resolver_ip);
@@ -323,6 +334,9 @@ int main(int argc, char *argv[]) {
         break;
       case URL_TYPE_IP:
         doh_proxy_set_port(proxy, port);
+        if (opt.resolver_ip != NULL) {
+          WLOG("Resolver URL already contains an IP address, ignoring -R %s", opt.resolver_ip);
+        }
         ILOG("Resolver prefix '%s' doesn't appear to contain a hostname. "
              "DNS polling disabled.", opt.resolver_url);
         systemd_notify_ready();

src/options.h

@@ -66,7 +66,7 @@ struct Options {
 
   // Number of logs to be kept by flight recorder
   int flight_recorder_size;
-} __attribute__((aligned(128)));
+};
 typedef struct Options options_t;
 
 enum OptionsParseResult {

src/ring_buffer.c

@@ -15,7 +15,7 @@ struct ring_buffer
     uint32_t size;
     uint32_t next;  // next slot to use in storage
     uint8_t full;
-} __attribute__((packed)) __attribute__((aligned(32)));
+};
 
 void ring_buffer_init(struct ring_buffer **rbp, uint32_t size)
 {