Tweak NTP error reporting

src/FTL.h

@@ -165,7 +165,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__)

src/api/auth.c

@@ -110,7 +110,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

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)
 			{

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

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_err("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

src/dnsmasq_interface.c

@@ -1823,12 +1823,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)

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;
@@ -52,9 +65,32 @@ 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 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 +113,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] = { 0 };
+		if(getnameinfo(saddr->ai_addr, saddr->ai_addrlen, ip, sizeof(ip), NULL, 0, NI_NUMERICHOST) != 0)
+			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));
 		return false;
 	}
 
@@ -214,16 +255,21 @@ 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(const int fd, const char *server, struct addrinfo *saddr, struct ntp_sync *ntp)
 {
 	// NTP Packet buffer
 	unsigned char buf[48];
 
 	// Receive reply
-	if(recv(fd, buf, 48, 0) < 48)
+	if(recv_nowarn(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] = { 0 };
+		if(getnameinfo(saddr->ai_addr, saddr->ai_addrlen, ip, sizeof(ip), NULL, 0, NI_NUMERICHOST) != 0)
+			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));
 		return false;
 	}
 
@@ -247,6 +293,10 @@ static bool reply(int fd, const char *server, struct ntp_sync *ntp, const bool v
 	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)
@@ -279,6 +329,17 @@ static bool reply(int fd, const char *server, struct ntp_sync *ntp, const bool v
 	// 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;
 	}
@@ -425,15 +486,15 @@ 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);
 			freeaddrinfo(saddr);
 			return false;
 		}
 		// Get reply
-		if(!reply(s, server, &ntp[i], false))
+		if(!reply(s, server, saddr, &ntp[i]))
 		{
 			close(s);
 			continue;
@@ -583,17 +644,19 @@ 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)
 {
+	(void)arg;
 	// Set thread name
 	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)
@@ -618,22 +681,37 @@ 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
+			{
+
+				// Reduce retry time if the time is not accurate enough
+				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);
+			}
 		}
 
 		// 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");

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");
 	}
 }
 
@@ -302,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));