From 2150b18bc05d785d02e1adbb083c161389c8b600 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 30 Nov 2024 18:40:16 +0100 Subject: [PATCH 01/14] Include used IP address of resolved NTP server in connection errors Signed-off-by: DL6ER --- src/ntp/client.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index 340358bf3..595b9f612 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -54,7 +54,7 @@ struct ntp_sync // Create minimal NTP request, see server implementation for details about the // packet structure -static bool request(int fd, const char *server, struct ntp_sync *ntp) +static bool request(int fd, const char *server, struct addrinfo *saddr, struct ntp_sync *ntp) { // NTP Packet buffer unsigned char buf[48] = {0}; @@ -77,8 +77,13 @@ static bool request(int fd, const char *server, struct ntp_sync *ntp) // Send request if(send(fd, buf, 48, 0) != 48) { - log_err("Failed to send data to NTP server %s: %s", - server, errno == EAGAIN ? "Timeout" : strerror(errno)); + // Get IP address of server + char ip[INET6_ADDRSTRLEN]; + if(getnameinfo(saddr->ai_addr, saddr->ai_addrlen, ip, sizeof(ip), NULL, 0, NI_NUMERICHOST) != 0) + strncpy(ip, server, sizeof(ip)); + + log_err("Failed to send data to NTP server %s (%s): %s", + server, ip, errno == EAGAIN ? "Timeout" : strerror(errno)); return false; } @@ -214,7 +219,7 @@ static bool settime_skew(const double offset) return true; } -static bool reply(int fd, const char *server, struct ntp_sync *ntp, const bool verbose) +static bool reply(int fd, const char *server, struct addrinfo *saddr, struct ntp_sync *ntp, const bool verbose) { // NTP Packet buffer unsigned char buf[48]; @@ -222,8 +227,13 @@ static bool reply(int fd, const char *server, struct ntp_sync *ntp, const bool v // Receive reply if(recv(fd, buf, 48, 0) < 48) { - log_err("Failed to receive data from NTP server %s: %s", - server, errno == EAGAIN ? "Timeout" : strerror(errno)); + // Get IP address of server + char ip[INET6_ADDRSTRLEN]; + if(getnameinfo(saddr->ai_addr, saddr->ai_addrlen, ip, sizeof(ip), NULL, 0, NI_NUMERICHOST) != 0) + strncpy(ip, server, sizeof(ip)); + + log_err("Failed to receive data from NTP server %s (%s): %s", + server, ip, errno == EAGAIN ? "Timeout" : strerror(errno)); return false; } @@ -425,7 +435,7 @@ bool ntp_client(const char *server, const bool settime, const bool print) continue; // Send request - if(!request(s, server, &ntp[i])) + if(!request(s, server, saddr, &ntp[i])) { close(s); free(ntp); @@ -433,7 +443,7 @@ bool ntp_client(const char *server, const bool settime, const bool print) return false; } // Get reply - if(!reply(s, server, &ntp[i], false)) + if(!reply(s, server, saddr, &ntp[i], false)) { close(s); continue; From 7efff4f43582a7b850af3c57799cbe89735cc90a Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 30 Nov 2024 18:44:55 +0100 Subject: [PATCH 02/14] Do not issue double warnings when receiving NTP replies fails Signed-off-by: DL6ER --- src/FTL.h | 3 ++- src/ntp/client.c | 2 +- src/syscalls/recv.c | 4 ++-- src/syscalls/syscalls.h | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/FTL.h b/src/FTL.h index c40d8e525..e9ec5cdd2 100644 --- a/src/FTL.h +++ b/src/FTL.h @@ -167,7 +167,8 @@ #define vsnprintf(buffer, maxlen, format, args) FTLvsnprintf(__FILE__, __FUNCTION__, __LINE__, buffer, maxlen, format, args) #define write(fd, buf, n) FTLwrite(fd, buf, n, __FILE__, __FUNCTION__, __LINE__) #define accept(sockfd, addr, addrlen) FTLaccept(sockfd, addr, addrlen, __FILE__, __FUNCTION__, __LINE__) -#define recv(sockfd, buf, len, flags) FTLrecv(sockfd, buf, len, flags, __FILE__, __FUNCTION__, __LINE__) +#define recv(sockfd, buf, len, flags) FTLrecv(sockfd, buf, len, flags, true, __FILE__, __FUNCTION__, __LINE__) +#define recv_nowarn(sockfd, buf, len, flags) FTLrecv(sockfd, buf, len, flags,false, __FILE__, __FUNCTION__, __LINE__) #define recvfrom(sockfd, buf, len, flags, src_addr, addrlen) FTLrecvfrom(sockfd, buf, len, flags, src_addr, addrlen, __FILE__, __FUNCTION__, __LINE__) #define sendto(sockfd, buf, len, flags, dest_addr, addrlen) FTLsendto(sockfd, buf, len, flags, dest_addr, addrlen, __FILE__, __FUNCTION__, __LINE__) #define select(nfds, readfds, writefds, exceptfds, timeout) FTLselect(nfds, readfds, writefds, exceptfds, timeout, __FILE__, __FUNCTION__, __LINE__) diff --git a/src/ntp/client.c b/src/ntp/client.c index 595b9f612..0e09c3e9e 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -225,7 +225,7 @@ static bool reply(int fd, const char *server, struct addrinfo *saddr, struct ntp unsigned char buf[48]; // Receive reply - if(recv(fd, buf, 48, 0) < 48) + if(recv_nowarn(fd, buf, 48, 0) < 48) { // Get IP address of server char ip[INET6_ADDRSTRLEN]; diff --git a/src/syscalls/recv.c b/src/syscalls/recv.c index 0dce546dc..92b2c4b32 100644 --- a/src/syscalls/recv.c +++ b/src/syscalls/recv.c @@ -15,7 +15,7 @@ #include #undef recv -ssize_t FTLrecv(int sockfd, void *buf, size_t len, int flags, const char *file, const char *func, const int line) +ssize_t FTLrecv(int sockfd, void *buf, size_t len, int flags, const bool warn, const char *file, const char *func, const int line) { ssize_t ret = 0; do @@ -33,7 +33,7 @@ ssize_t FTLrecv(int sockfd, void *buf, size_t len, int flags, const char *file, // Final error checking (may have failed for some other reason then an // EINTR = interrupted system call) - if(ret < 0) + if(warn && ret < 0) log_warn("Could not recv() in %s() (%s:%i): %s", func, file, line, strerror(errno)); diff --git a/src/syscalls/syscalls.h b/src/syscalls/syscalls.h index 02024a844..68caa9284 100644 --- a/src/syscalls/syscalls.h +++ b/src/syscalls/syscalls.h @@ -36,7 +36,7 @@ int FTLvsnprintf(const char *file, const char *func, const int line, char *__res // Interrupt-safe socket routines ssize_t FTLwrite(int fd, const void *buf, size_t total, const char *file, const char *func, const int line); int FTLaccept(int sockfd, struct sockaddr *addr, socklen_t *addrlen, const char *file, const char *func, const int line); -ssize_t FTLrecv(int sockfd, void *buf, size_t len, int flags, const char *file, const char *func, const int line); +ssize_t FTLrecv(int sockfd, void *buf, size_t len, int flags, const bool warn, const char *file, const char *func, const int line); ssize_t FTLrecvfrom(int sockfd, void *buf, size_t len, int flags, struct sockaddr *src_addr, socklen_t *addrlen, const char *file, const char *func, const int line); int FTLselect(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, struct timeval *timeout, const char *file, const char *func, const int line); ssize_t FTLsendto(int sockfd, void *buf, size_t len, int flags, const struct sockaddr *dest_addr, socklen_t addrlen, const char *file, const char *func, const int line); From 7c6e315bdfd347d065d06a08929e5c4f4066dd51 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 30 Nov 2024 18:54:43 +0100 Subject: [PATCH 03/14] Implement Kiss Code interpretation Signed-off-by: DL6ER --- src/ntp/client.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/ntp/client.c b/src/ntp/client.c index 0e09c3e9e..04bbd3f71 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -52,6 +52,29 @@ struct ntp_sync double precision; }; +// Kiss codes as defined in RFC 5905, Section 7.4 +static struct { + const char *code; + const char *meaning; +} kiss_codes[] = +{ + { "ACST", "The association belongs to a unicast server." }, + { "AUTH", "Server authentication failed." }, + { "AUTO", "Autokey sequence failed." }, + { "BCST", "The association belongs to a broadcast server." }, + { "CRYP", "Cryptographic authentication or identification failed." }, + { "DENY", "Access denied by remote server." }, + { "DROP", "Lost peer in symmetric mode." }, + { "RSTR", "Access denied due to local policy." }, + { "INIT", "The association has not yet synchronized for the first time." }, + { "MCST", "The association belongs to a dynamically discovered server." }, + { "NKEY", "No key found. Either the key was never installed or is not trusted." }, + { "RATE", "Rate exceeded. The server has temporarily denied access because the client exceeded the rate threshold." }, + { "RMOT", "Alteration of association from a remote host running ntpdc." }, + { "STEP", "A step change in system time has occurred, but the association has not yet resynchronized." }, + { NULL, NULL } +}; + // Create minimal NTP request, see server implementation for details about the // packet structure static bool request(int fd, const char *server, struct addrinfo *saddr, struct ntp_sync *ntp) @@ -257,6 +280,10 @@ static bool reply(int fd, const char *server, struct addrinfo *saddr, struct ntp memcpy(&srv_root_delay, &buf[4], sizeof(srv_root_delay)); memcpy(&srv_root_dispersion, &buf[8], sizeof(srv_root_dispersion)); + // Extract reference ID (Kiss code) + char kiss_code[4]; + memcpy(kiss_code, &buf[12], sizeof(kiss_code)); + // Extract Transmit Timestamp uint64_t netbuffer; // ref = Reference Timestamp (Time at which the clock was last set or corrected) @@ -289,6 +316,17 @@ static bool reply(int fd, const char *server, struct addrinfo *saddr, struct ntp // Check stratum, mode, version, etc. if((buf[0] & 0x07) != 4) { + // Check for possible Kiss code + for(size_t i = 0; kiss_codes[i].code != NULL; i++) + { + if(memcmp(kiss_code, kiss_codes[i].code, sizeof(kiss_code)) == 0) + { + log_warn("Received NTP reply has Kiss code %s: %s, ignoring", + kiss_codes[i].code, kiss_codes[i].meaning); + return false; + } + } + // else: log_warn("Received NTP reply has invalid version, ignoring"); return false; } From a76e79186f0801d4ea290db45d89c627cae988fb Mon Sep 17 00:00:00 2001 From: DL6ER Date: Fri, 13 Dec 2024 21:04:32 +0100 Subject: [PATCH 04/14] Do not fork when handling NTP replies Signed-off-by: DL6ER --- src/ntp/server.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/ntp/server.c b/src/ntp/server.c index 7c89fd07c..f31ff5987 100644 --- a/src/ntp/server.c +++ b/src/ntp/server.c @@ -266,17 +266,9 @@ static void request_process_loop(const int fd, const char *ipstr, const int prot } } - // Fork a child to handle the request - const pid_t pid = fork(); - if (pid == 0) { - // Child - ntp_reply(fd, &src_addr , src_addrlen, buf, &recv_time); - exit(0); - } else if (pid == -1) { - log_err("fork() error"); - return; - } - // return to parent + // Handle the request + ntp_reply(fd, &src_addr, src_addrlen, buf, &recv_time); + log_debug(DEBUG_NTP, "NTP reply sent"); } } From 5af65fc3564435512ca169496403afa139e303d1 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 17 Dec 2024 20:49:59 +0100 Subject: [PATCH 05/14] Improve error checking in several functions of resolve.c Signed-off-by: DL6ER --- src/api/auth.c | 2 +- src/api/config.c | 2 +- src/api/dns.c | 10 ++--- src/dnsmasq/helper.c | 11 +++-- src/resolve.c | 95 +++++++++++++++++++++++++++++++++----------- src/timers.c | 1 + src/timers.h | 2 + 7 files changed, 88 insertions(+), 35 deletions(-) diff --git a/src/api/auth.c b/src/api/auth.c index 22bbd9de2..fd28d3fea 100644 --- a/src/api/auth.c +++ b/src/api/auth.c @@ -106,7 +106,7 @@ int check_client_auth(struct ftl_conn *api, const bool is_api) // Try to extract SID from root of a possibly included JSON payload else if(api->payload.json != NULL) { - cJSON *sid_obj = cJSON_GetObjectItem(api->payload.json, "sid"); + const cJSON *sid_obj = cJSON_GetObjectItem(api->payload.json, "sid"); if(cJSON_IsString(sid_obj)) { // Copy SID string diff --git a/src/api/config.c b/src/api/config.c index 690f81039..1fc1e08cb 100644 --- a/src/api/config.c +++ b/src/api/config.c @@ -959,7 +959,7 @@ static int api_config_put_delete(struct ftl_conn *api) int idx = 0; for(; idx < cJSON_GetArraySize(new_item->v.json); idx++) { - cJSON *elem = cJSON_GetArrayItem(new_item->v.json, idx); + const cJSON *elem = cJSON_GetArrayItem(new_item->v.json, idx); if(elem != NULL && elem->valuestring != NULL && strcmp(elem->valuestring, new_item_str) == 0) { diff --git a/src/api/dns.c b/src/api/dns.c index a7747868d..f3d5e56eb 100644 --- a/src/api/dns.c +++ b/src/api/dns.c @@ -77,17 +77,17 @@ static int set_blocking(struct ftl_conn *api) const enum blocking_status target_status = cJSON_IsTrue(elem) ? BLOCKING_ENABLED : BLOCKING_DISABLED; // Get (optional) timer - double timer = -1; + double time = -1; elem = cJSON_GetObjectItemCaseSensitive(api->payload.json, "timer"); if (cJSON_IsNumber(elem) && elem->valuedouble > 0.0) - timer = elem->valuedouble; + time = elem->valuedouble; if(target_status == get_blockingstatus()) { // The blocking status does not need to be changed // Delete a possibly running timer - set_blockingmode_timer(timer, true); + set_blockingmode_timer(time, true); log_debug(DEBUG_API, "No change in blocking mode, resetting timer"); } @@ -97,9 +97,9 @@ static int set_blocking(struct ftl_conn *api) set_blockingstatus(target_status); // Start timer (-1 disables all running timers) - set_blockingmode_timer(timer, !target_status); + set_blockingmode_timer(time, !target_status); - log_debug(DEBUG_API, "%sd Pi-hole, timer set to %f seconds", target_status ? "Enable" : "Disable", timer); + log_debug(DEBUG_API, "%sd Pi-hole, timer set to %f seconds", target_status ? "Enable" : "Disable", time); } // Return GET property as result of POST/PUT/PATCH action diff --git a/src/dnsmasq/helper.c b/src/dnsmasq/helper.c index 65727ba4a..4c7c355a2 100644 --- a/src/dnsmasq/helper.c +++ b/src/dnsmasq/helper.c @@ -98,10 +98,6 @@ int create_helper(int event_fd, int err_fd, uid_t uid, gid_t gid, long max_fd) return pipefd[1]; } - /**** Pi-hole modification ****/ - log_info("Started script helper"); - /******************************/ - /* ignore SIGTERM and SIGINT, so that we can clean up when the main process gets hit and SIGALRM so that we can use sleep() */ sigact.sa_handler = SIG_IGN; @@ -128,10 +124,17 @@ int create_helper(int event_fd, int err_fd, uid_t uid, gid_t gid, long max_fd) /* return error */ send_event(err_fd, EVENT_USER_ERR, errno, daemon->scriptuser); } + /**** Pi-hole modification ****/ + log_info("Starting script helper FAILED"); + /******************************/ _exit(0); } } + /**** Pi-hole modification ****/ + log_info("Started script helper"); + /******************************/ + /* close all the sockets etc, we don't need them here. Don't close err_fd, in case the lua-init fails. Note that we have to do this before lua init diff --git a/src/resolve.c b/src/resolve.c index c60797482..094cc5f06 100644 --- a/src/resolve.c +++ b/src/resolve.c @@ -37,8 +37,8 @@ #include "dnsmasq/config.h" // Function Prototypes -static void name_toDNS(unsigned char *dns, const size_t dnslen, const char *host, const size_t hostlen) __attribute__((nonnull(1,3))); -static unsigned char *name_fromDNS(unsigned char *reader, unsigned char *buffer, uint16_t *count) __attribute__((malloc)) __attribute__((nonnull(1,2,3))); +static void nameToDNS(unsigned char *dns, const size_t dnslen, const char *host, const size_t hostlen) __attribute__((nonnull(1,3))); +static unsigned char *nameFromDNS(unsigned char *reader, unsigned char *buffer, uint16_t *count) __attribute__((malloc)) __attribute__((nonnull(1,2,3))); // Avoid "error: packed attribute causes inefficient alignment for ..." on ARM32 // builds due to the use of __attribute__((packed)) in the following structs @@ -329,7 +329,7 @@ static char *__attribute__((malloc)) ngethostbyname(const int sock, const bool t strncat(hname, ".", hnamelen - strlen(hname)); hname[hnamelen - 1] = '\0'; - name_toDNS(qname, sizeof(buf) - sizeof(struct DNS_HEADER), hname, hnamelen); + nameToDNS(qname, sizeof(buf) - sizeof(struct DNS_HEADER), hname, hnamelen); free(hname); qinfo = (void*)&buf[sizeof(struct DNS_HEADER) + (strlen((const char*)qname) + 1)]; @@ -427,14 +427,14 @@ static char *__attribute__((malloc)) ngethostbyname(const int sock, const bool t char *name = NULL; for(uint16_t i = 0; i < min(ntohs(dns->ans_count), ArraySize(answers)); i++) { - answers[i].name = name_fromDNS(reader, buf, &stop); + answers[i].name = nameFromDNS(reader, buf, &stop); reader = reader + stop; answers[i].resource = (struct R_DATA*)(reader); reader = reader + sizeof(struct R_DATA); // Read the answer and convert from network to host representation - answers[i].rdata = name_fromDNS(reader, buf, &stop); + answers[i].rdata = nameFromDNS(reader, buf, &stop); reader = reader + stop; // We only care about PTR answers and ignore all others @@ -485,12 +485,17 @@ static char *__attribute__((malloc)) ngethostbyname(const int sock, const bool t // Convert hostname from network to host representation // This routine supports DNS compression pointers // 3www6google3com -> www.google.com -static u_char * __attribute__((malloc)) __attribute__((nonnull(1,2,3))) name_fromDNS(unsigned char *reader, unsigned char *buffer, uint16_t *count) +static u_char * __attribute__((malloc)) __attribute__((nonnull(1,2,3))) nameFromDNS(unsigned char *reader, unsigned char *buffer, uint16_t *count) { const size_t MAXNAMELEN = 256; unsigned char *name = calloc(MAXNAMELEN, sizeof(char)); - unsigned int p = 0, jumped = 0; + if(name == NULL) + { + log_err("Unable to allocate memory in nameFromDNS"); + return NULL; + } + unsigned int p = 0, jumped = 0; // Initialize count *count = 1; @@ -530,6 +535,12 @@ static u_char * __attribute__((malloc)) __attribute__((nonnull(1,2,3))) name_fro // restricted to 63 octets or less. See the referenced // RFC for more details. const unsigned int offset = ((*reader - 0xC0) << 8) + *(reader + 1); + if(offset >= MAXNAMELEN) + { + log_err("DNS compression pointer out of bounds: %u", offset); + free(name); + return NULL; + } reader = buffer + offset - 1; jumped = 1; // We have jumped to another location so counting won't go up } @@ -573,26 +584,28 @@ static u_char * __attribute__((malloc)) __attribute__((nonnull(1,2,3))) name_fro // www.google.com -> 3www6google3com // We do not use DNS compression pointers here as we do not know if the DNS // server we are talking to supports them -static void __attribute__((nonnull(1,3))) name_toDNS(unsigned char *dns, const size_t dnslen, const char *host, const size_t hostlen) +static void __attribute__((nonnull(1,3))) nameToDNS(unsigned char *dns, const size_t dnslen, const char *host, const size_t hostlen) { unsigned int lock = 0; + const unsigned char *dns_start = dns; - // Iterate over hostname characters - for(unsigned int i = 0 ; i < strlen((char*)host); i++) + // Iterate over hostname characters and convert to DNS format + // Also check for buffer overflow of the DNS buffer + for(unsigned int i = 0; i < hostlen && (const size_t)(dns - dns_start) < dnslen; i++) { // If we encounter a dot, write the number of characters since the last dot // and then write the characters themselves if(host[i] == '.') { *dns++ = i - lock; - for(;lock < i; lock++) + for(;lock < i && (const size_t)(dns - dns_start) < dnslen; lock++) *dns++ = host[lock]; lock++; } } // Terminate the string at the end - *dns++='\0'; + *dns++ = '\0'; } char *__attribute__((malloc)) resolveHostname(const int sock, const bool tcp, struct sockaddr_in *dest, @@ -607,7 +620,10 @@ char *__attribute__((malloc)) resolveHostname(const int sock, const bool tcp, st log_debug(DEBUG_RESOLVER, "Configured to not resolve host name for %s", addr); // Return an empty host name - return strdup(""); + hostn = strdup(""); + if(hostn == NULL) + log_err("Unable to allocate memory for empty host name"); + return hostn; } log_debug(DEBUG_RESOLVER, "Trying to resolve %s", addr); @@ -617,6 +633,8 @@ char *__attribute__((malloc)) resolveHostname(const int sock, const bool tcp, st if(strcmp(addr, "0.0.0.0") == 0) { hostn = strdup("hidden"); + if(hostn == NULL) + log_err("Unable to allocate memory for hidden host name"); log_debug(DEBUG_RESOLVER, "---> \"%s\" (privacy settings)", hostn); return hostn; } @@ -626,6 +644,8 @@ char *__attribute__((malloc)) resolveHostname(const int sock, const bool tcp, st if(strcmp(addr, "::") == 0) { hostn = strdup("pi.hole"); + if(hostn == NULL) + log_err("Unable to allocate memory for internal host name"); log_debug(DEBUG_RESOLVER, "---> \"%s\" (special)", hostn); return hostn; } @@ -696,7 +716,10 @@ char *__attribute__((malloc)) resolveHostname(const int sock, const bool tcp, st if(!inet_pton(ss.ss_family, addr, &(((struct sockaddr_in *)&ss)->sin_addr))) { log_warn("Invalid IPv4 address when trying to resolve hostname: %s", addr); - return strdup(""); + hostn = strdup(""); + if(hostn == NULL) + log_err("Unable to allocate memory for empty host name"); + return hostn; } // Need extra space for ".in-addr.arpa" suffix @@ -739,6 +762,21 @@ static size_t resolveAndAddHostname(const int udp_sock, struct sockaddr_in *dest char *oldname = strdup(getstr(oldnamepos)); unlock_shm(); + if(ipaddr == NULL || oldname == NULL) + { + log_err("Unable to allocate memory for IP address or host name"); + + // Free allocated memory + if(ipaddr != NULL) + free(ipaddr); + if(oldname != NULL) + free(oldname); + + // Return old namepos + *success = false; + return oldnamepos; + } + // Test if we want to resolve host names, otherwise all calls to resolveHostname() // and getNameFromIP() can be skipped as they will all return empty names (= no records) if(!resolve_this_name(ipaddr)) @@ -761,15 +799,22 @@ static size_t resolveAndAddHostname(const int udp_sock, struct sockaddr_in *dest { // Retry with TCP if UDP failed due to truncation (RFC 7766) const int tcp_sock = create_socket(true, dest); - newname = resolveHostname(tcp_sock, true, dest, ipaddr, false, NULL); - close(tcp_sock); + if(tcp_sock > 0) + { + // Only attempt to resolve the hostname if we have a + // valid socket + newname = resolveHostname(tcp_sock, true, dest, ipaddr, false, NULL); + close(tcp_sock); + } + else + log_warn("Unable to create TCP socket for DNS resolution"); } if(newname == NULL) { // We could not resolve the hostname, so we keep the old one // and mark the entry as not new - log_debug(DEBUG_RESOLVER, " ---> \"%s\" (failed to resolve via UDP, too)", oldname); + log_debug(DEBUG_RESOLVER, " ---> \"%s\" (failed to resolve via TCP, too)", oldname); // Free allocated memory *success = false; @@ -794,7 +839,8 @@ static size_t resolveAndAddHostname(const int udp_sock, struct sockaddr_in *dest if(newname != NULL && strcmp(oldname, newname) != 0) { lock_shm(); - size_t newnamepos = addstr(newname); + const size_t newnamepos = addstr(newname); + unlock_shm(); // Free allocated memory // newname has already been checked against NULL @@ -802,7 +848,6 @@ static size_t resolveAndAddHostname(const int udp_sock, struct sockaddr_in *dest free(newname); free(ipaddr); free(oldname); - unlock_shm(); return newnamepos; } else @@ -826,7 +871,7 @@ static void resolveClients(const bool onlynew, const bool force_refreshing) const time_t now = time(NULL); // Lock counter access here, we use a copy in the following loop lock_shm(); - unsigned int clientscount = counters->clients; + const unsigned int clientscount = counters->clients; unlock_shm(); // Create DNS client socket @@ -889,13 +934,15 @@ static void resolveClients(const bool onlynew, const bool force_refreshing) continue; } + // Get IP address of client + const char *ipaddr = getstr(ippos); + unlock_shm(); + // Check if we want to resolve an IPv6 address bool IPv6 = false; - const char *ipaddr = NULL; - if((ipaddr = getstr(ippos)) != NULL && strstr(ipaddr,":") != NULL) + if(strstr(ipaddr, ":") != NULL) IPv6 = true; - unlock_shm(); // If we're in refreshing mode (onlynew == false), we skip clients if // 1. We should not refresh any hostnames @@ -981,7 +1028,7 @@ static void resolveUpstreams(const bool onlynew) const time_t now = time(NULL); // Lock counter access here, we use a copy in the following loop lock_shm(); - int upstreams = counters->upstreams; + const int upstreams = counters->upstreams; unlock_shm(); // Create socket diff --git a/src/timers.c b/src/timers.c index b87c8c929..6a4351d96 100644 --- a/src/timers.c +++ b/src/timers.c @@ -83,6 +83,7 @@ void get_blockingmode_timer(double *delay, bool *target_status) #define SLEEPING_TIME 0.1 // seconds void *timer(void *val) { + (void)val; // Set thread name prctl(PR_SET_NAME, thread_names[TIMER], 0, 0, 0); diff --git a/src/timers.h b/src/timers.h index 7879371dc..769c8c8ad 100644 --- a/src/timers.h +++ b/src/timers.h @@ -12,6 +12,8 @@ #include "enums.h" +#include + #define NUMTIMERS LAST_TIMER void timer_start(const enum timers i); From 9cd11375de872e02e997425bc57502089e54e614 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 17 Dec 2024 21:34:02 +0100 Subject: [PATCH 06/14] Minor further enhancements for NTP code Signed-off-by: DL6ER --- src/ntp/client.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index 4ca65e82e..751471126 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -101,9 +101,9 @@ static bool request(int fd, const char *server, struct addrinfo *saddr, struct n if(send(fd, buf, 48, 0) != 48) { // Get IP address of server - char ip[INET6_ADDRSTRLEN]; + char ip[INET6_ADDRSTRLEN] = { 0 }; if(getnameinfo(saddr->ai_addr, saddr->ai_addrlen, ip, sizeof(ip), NULL, 0, NI_NUMERICHOST) != 0) - strncpy(ip, server, sizeof(ip)); + strncpy(ip, server, sizeof(ip) - 1); log_err("Failed to send data to NTP server %s (%s): %s", server, ip, errno == EAGAIN ? "Timeout" : strerror(errno)); @@ -242,7 +242,7 @@ static bool settime_skew(const double offset) return true; } -static bool reply(int fd, const char *server, struct addrinfo *saddr, struct ntp_sync *ntp, const bool verbose) +static bool reply(const int fd, const char *server, struct addrinfo *saddr, struct ntp_sync *ntp) { // NTP Packet buffer unsigned char buf[48]; @@ -251,9 +251,9 @@ static bool reply(int fd, const char *server, struct addrinfo *saddr, struct ntp if(recv_nowarn(fd, buf, 48, 0) < 48) { // Get IP address of server - char ip[INET6_ADDRSTRLEN]; + char ip[INET6_ADDRSTRLEN] = { 0 }; if(getnameinfo(saddr->ai_addr, saddr->ai_addrlen, ip, sizeof(ip), NULL, 0, NI_NUMERICHOST) != 0) - strncpy(ip, server, sizeof(ip)); + strncpy(ip, server, sizeof(ip) - 1); log_err("Failed to receive data from NTP server %s (%s): %s", server, ip, errno == EAGAIN ? "Timeout" : strerror(errno)); @@ -481,7 +481,7 @@ bool ntp_client(const char *server, const bool settime, const bool print) return false; } // Get reply - if(!reply(s, server, saddr, &ntp[i], false)) + if(!reply(s, server, saddr, &ntp[i])) { close(s); continue; @@ -638,6 +638,7 @@ bool ntp_client(const char *server, const bool settime, const bool print) static void *ntp_client_thread(void *arg) { + (void)arg; // Set thread name prctl(PR_SET_NAME, thread_names[NTP_CLIENT], 0, 0, 0); From cf2c7c36dfbb1d2098d6877b17827fb4ee69a818 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Wed, 18 Dec 2024 20:41:59 +0100 Subject: [PATCH 07/14] Ensure NTP port can be re-bound even after fast successive restarts Signed-off-by: DL6ER --- src/ntp/server.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/ntp/server.c b/src/ntp/server.c index f31ff5987..a78240dfc 100644 --- a/src/ntp/server.c +++ b/src/ntp/server.c @@ -294,6 +294,15 @@ static void *ntp_bind_and_listen(void *param) return NULL; } +#ifdef SO_REUSEPORT + // Set socket option to allow multiple sockets to bind to the same port + // (SO_REUSEPORT). This is necessary to ensure pihole-FTL can rebind to + // the NTP port after a restart without waiting for the kernel to release + // the port. This feature is available since Linux 3.9. + int optval = 1; + setsockopt(s, SOL_SOCKET, SO_REUSEPORT, &optval, sizeof(optval)); +#endif + // Bind the socket to the NTP port char ipstr[INET6_ADDRSTRLEN + 1]; memset(ipstr, 0, sizeof(ipstr)); From 747b89987f97d5a6afe65b19e631a1c5b788a21e Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 19 Dec 2024 09:37:10 +0100 Subject: [PATCH 08/14] Do not try to obtain -1 ID pointer in CNAME code to prevent harmless ERROR message from showing up in the logs Signed-off-by: DL6ER --- src/dnsmasq_interface.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c index 47bafb146..82d2caad9 100644 --- a/src/dnsmasq_interface.c +++ b/src/dnsmasq_interface.c @@ -1805,12 +1805,12 @@ bool FTL_CNAME(const char *dst, const char *src, const int id) else if(query->status == QUERY_REGEX) { // Get parent and child DNS cache entries - const unsigned int parent_cacheID = query->cacheID > -1 ? query->cacheID : findCacheID(parent_domainID, clientID, query->type, false); - const unsigned int child_cacheID = findCacheID(child_domainID, clientID, query->type, false); + const int parent_cacheID = query->cacheID > -1 ? query->cacheID : findCacheID(parent_domainID, clientID, query->type, false); + const int child_cacheID = findCacheID(child_domainID, clientID, query->type, false); // Get cache pointers - DNSCacheData *parent_cache = getDNSCache(parent_cacheID, true); - const DNSCacheData *child_cache = getDNSCache(child_cacheID, true); + DNSCacheData *parent_cache = parent_cacheID < 0 ? NULL : getDNSCache(parent_cacheID, true); + const DNSCacheData *child_cache = child_cacheID < 0 ? NULL : getDNSCache(child_cacheID, true); // Propagate ID of responsible regex up from the child to the parent // domain (but only if set) From c002dc2cb73c938d7c8668d2abeebb13d3f8f16f Mon Sep 17 00:00:00 2001 From: DL6ER Date: Fri, 20 Dec 2024 20:03:02 +0100 Subject: [PATCH 09/14] Dynamically reduce retry interval of NTP client in case the first sync was sufficiently far off (> 0.1 s) to postpone the start of the embedded NTP server Signed-off-by: DL6ER --- src/ntp/client.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index 751471126..9084c408f 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -643,6 +643,7 @@ static void *ntp_client_thread(void *arg) prctl(PR_SET_NAME, thread_names[NTP_CLIENT], 0, 0, 0); // Run NTP client + unsigned int retry_count = 0; bool ntp_server_started = false; bool first_run = true; while(!killed) @@ -667,22 +668,36 @@ static void *ntp_client_thread(void *arg) restart_ftl("System time updated"); } + // Calculate time to sleep + unsigned int sleep_time = config.ntp.sync.interval.v.ui - (unsigned int)time_delta; + // Set first run to false first_run = false; - if(success && !ntp_server_started) + if(!ntp_server_started) { - // Initialize NTP server only after first NTP - // synchronization to ensure that the time is set - // correctly - ntp_server_started = ntp_server_start(); + if(success) + { + // Initialize NTP server only after first high + // accuracy NTP synchronization to ensure that + // the time is set correctly + ntp_server_started = ntp_server_start(); + } + else + { + log_debug(DEBUG_NTP, "Local time is too inaccurate, retrying before launching NTP server"); + + // Reduce retry time to 10 seconds (at most three times) + if(retry_count++ < 3) + sleep_time = 10; + } } // Intermediate cancellation-point BREAK_IF_KILLED(); // Sleep before retrying - thread_sleepms(NTP_CLIENT, 1000 * config.ntp.sync.interval.v.ui); + thread_sleepms(NTP_CLIENT, 1000 * sleep_time); } log_info("Terminating NTP thread"); From bc67a9f697a15dc201d5af4049313dbbc31ca333 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 21 Dec 2024 08:39:21 +0100 Subject: [PATCH 10/14] Reduce time accuracy requirements for NTP service to 0.5 seconds (compile-time constant) and enlarge retry interval to 10 minutes (or ntp.sync.interval if smaller) Signed-off-by: DL6ER --- src/ntp/client.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index 9084c408f..d93418bb9 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -42,6 +42,19 @@ // check_capability() #include "capabilities.h" +// Required accuracy of the NTP sync in seconds in order to start the NTP server +// thread. If the NTP sync is less accurate than this value, the NTP server +// thread will only be started after later NTP syncs have reached this accuracy. +// Default: 0.5 (seconds) +#define ACCURACY 0.5 + +// Interval between successive NTP sync attempts in seconds in case of +// not-yet-sufficient accuracy of the NTP sync +// Default: 600 (seconds) = 10 minutes +#define RETRY_INTERVAL 600 +// Maximum number of NTP syncs to attempt before giving up +#define RETRY_ATTEMPTS 5 + struct ntp_sync { bool valid; @@ -631,9 +644,9 @@ bool ntp_client(const char *server, const bool settime, const bool print) ntp_sync_rtc(); } - // Offset and delay larger than 0.1 seconds are considered as invalid + // Offset and delay larger than ACCURACY seconds are considered as invalid // during local testing (e.g., when the server is on the same machine) - return theta_avg < 0.1 && delta_avg < 0.1; + return theta_trim < ACCURACY && delta_trim < ACCURACY; } static void *ntp_client_thread(void *arg) @@ -687,9 +700,10 @@ static void *ntp_client_thread(void *arg) { log_debug(DEBUG_NTP, "Local time is too inaccurate, retrying before launching NTP server"); - // Reduce retry time to 10 seconds (at most three times) - if(retry_count++ < 3) - sleep_time = 10; + // Reduce retry time if the time is not accurate enough + if(retry_count++ < RETRY_INTERVAL && + sleep_time > RETRY_INTERVAL) + sleep_time = RETRY_INTERVAL; } } From a673d3f402c09fe18e25103ee6ce4db052ad87a9 Mon Sep 17 00:00:00 2001 From: Dominik Date: Sun, 22 Dec 2024 08:52:24 +0100 Subject: [PATCH 11/14] Update src/ntp/client.c Signed-off-by: Dominik --- src/ntp/client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index d93418bb9..bb11533a1 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -701,7 +701,7 @@ static void *ntp_client_thread(void *arg) log_debug(DEBUG_NTP, "Local time is too inaccurate, retrying before launching NTP server"); // Reduce retry time if the time is not accurate enough - if(retry_count++ < RETRY_INTERVAL && + if(retry_count++ < RETRY_ATTEMPTS && sleep_time > RETRY_INTERVAL) sleep_time = RETRY_INTERVAL; } From 0f090ad1eddcc30b02ae3d26ccfa19a0d9741bad Mon Sep 17 00:00:00 2001 From: Dominik Date: Sun, 12 Jan 2025 13:23:46 +0100 Subject: [PATCH 12/14] Update src/ntp/client.c Co-authored-by: yubiuser Signed-off-by: Dominik --- src/ntp/client.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ntp/client.c b/src/ntp/client.c index bb11533a1..0fe5c72a4 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -704,6 +704,7 @@ static void *ntp_client_thread(void *arg) if(retry_count++ < RETRY_ATTEMPTS && sleep_time > RETRY_INTERVAL) sleep_time = RETRY_INTERVAL; + log_info("Local time is too inaccurate, retrying in %u seconds before launching NTP server", sleep_time); } } From c5fe6494ff3bd1b4bdd14a972a975fc869b8362c Mon Sep 17 00:00:00 2001 From: Dominik Date: Sun, 12 Jan 2025 13:24:24 +0100 Subject: [PATCH 13/14] Apply suggestions from code review Co-authored-by: yubiuser Signed-off-by: Dominik --- src/dnsmasq/helper.c | 2 +- src/ntp/client.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/dnsmasq/helper.c b/src/dnsmasq/helper.c index 648fc6d10..1b7367760 100644 --- a/src/dnsmasq/helper.c +++ b/src/dnsmasq/helper.c @@ -125,7 +125,7 @@ int create_helper(int event_fd, int err_fd, uid_t uid, gid_t gid, long max_fd) send_event(err_fd, EVENT_USER_ERR, errno, daemon->scriptuser); } /**** Pi-hole modification ****/ - log_info("Starting script helper FAILED"); + log_err("Starting script helper FAILED"); /******************************/ _exit(0); } diff --git a/src/ntp/client.c b/src/ntp/client.c index 0fe5c72a4..a5fa12057 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -698,7 +698,6 @@ static void *ntp_client_thread(void *arg) } else { - log_debug(DEBUG_NTP, "Local time is too inaccurate, retrying before launching NTP server"); // Reduce retry time if the time is not accurate enough if(retry_count++ < RETRY_ATTEMPTS && From 8a26dd956ff9026d06fb6eae1534c0f50b265602 Mon Sep 17 00:00:00 2001 From: yubiuser Date: Sun, 12 Jan 2025 19:54:46 +0100 Subject: [PATCH 14/14] Fix space Signed-off-by: yubiuser --- src/ntp/client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index a5fa12057..7f6113825 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -703,7 +703,7 @@ static void *ntp_client_thread(void *arg) if(retry_count++ < RETRY_ATTEMPTS && sleep_time > RETRY_INTERVAL) sleep_time = RETRY_INTERVAL; - log_info("Local time is too inaccurate, retrying in %u seconds before launching NTP server", sleep_time); + log_info("Local time is too inaccurate, retrying in %u seconds before launching NTP server", sleep_time); } }