Merge pull request #1067 from pi-hole/fix/test_crash

Fix for `pihole-FTL test`

src/api/socket.c

@@ -281,23 +281,24 @@ static int listener(const int sockfd, const char type)
 void close_telnet_socket(void)
 {
  // Using global variable here
- if(telnetfd4)
+ if(telnetfd4 != 0)
  close(telnetfd4);
- if(telnetfd6)
+ if(telnetfd6 != 0)
  close(telnetfd6);
 }
 
 void close_unix_socket(bool unlink_file)
 {
+ // Using global variable here
+ if(sock_avail != 0)
+ close(socketfd);
+
+ // The process has to take care of unlinking the socket file description
+ // on exit
  if(unlink_file)
  {
- // The process has to take care of unlinking the socket file description on exit
  unlink(FTLfiles.socketfile);
  }
-
- // Using global variable here
- if(sock_avail)
- close(socketfd);
 }
 
 static void *telnet_connection_handler_thread(void *socket_desc)
@@ -420,8 +421,8 @@ void *telnet_listening_thread_IPv4(void *args)
  if(!ipv4telnet)
  return NULL;
 
- // Listen as long as FTL is not killed
- while(!killed)
+ // Listen as long as this thread is not canceled
+ while(true)
  {
  // Look for new clients that want to connect
  const int csck = listener(telnetfd4, 4);
@@ -469,8 +470,8 @@ void *telnet_listening_thread_IPv6(void *args)
  if(!ipv6telnet)
  return NULL;
 
- // Listen as long as FTL is not killed
- while(!killed)
+ // Listen as long as this thread is not canceled
+ while(true)
  {
  // Look for new clients that want to connect
  const int csck = listener(telnetfd6, 6);
@@ -518,8 +519,8 @@ void *socket_listening_thread(void *args)
  return NULL;
  }
 
- // Listen as long as FTL is not killed
- while(!killed)
+ // Listen as long as this thread is not canceled
+ while(true)
  {
  // Look for new clients that want to connect
  const int csck = listener(socketfd, 0);
@@ -540,7 +541,7 @@ void *socket_listening_thread(void *args)
  logg("WARNING: Unable to open socket processing thread: %s", strerror(errno));
  }
  }
- return false;
+ return NULL;
 }
 
 bool ipv6_available(void)

src/args.c

@@ -188,6 +188,9 @@ void parse_args(int argc, char* argv[])
  argv_dnsmasq[1] = "-d";
  }
 
+ // Full start FTL but shut down immediately once everything is up
+ // This ensures we'd catch any dnsmasq config errors,
+ // incorrect file permissions, etc.
  if(strcmp(argv[i], "test") == 0)
  {
  killed = 1;

src/daemon.c

@@ -14,6 +14,12 @@
 #include "log.h"
 // sleepms()
 #include "timers.h"
+// close_telnet_socket()
+#include "api/socket.h"
+// gravityDB_close()
+#include "database/gravity-db.h"
+// destroy_shmem()
+#include "shmem.h"
 
 bool resolver_ready = false;
 
@@ -99,10 +105,10 @@ void savepid(void)
  logg("PID of FTL process: %i", (int)pid);
 }
 
-void removepid(void)
+static void removepid(void)
 {
  FILE *f;
- if((f = fopen(FTLfiles.pid, "w+")) == NULL)
+ if((f = fopen(FTLfiles.pid, "w")) == NULL)
  {
  logg("WARNING: Unable to empty PID file");
  return;
@@ -158,3 +164,38 @@ pid_t FTL_gettid(void)
  return -1;
 #endif // SYS_gettid
 }
+
+// Clean up on exit
+void cleanup(const int ret)
+{
+ // Terminate threads before closing database connections and finishing shared memory
+ pthread_cancel(telnet_listenthreadv4);
+ pthread_cancel(telnet_listenthreadv6);
+ pthread_cancel(socket_listenthread);
+ pthread_cancel(DBthread);
+ pthread_cancel(GCthread);
+ pthread_cancel(DNSclientthread);
+
+ // Close gravity database connection
+ gravityDB_close();
+
+ // Close sockets and delete Unix socket file handle
+ close_telnet_socket();
+ close_unix_socket(true);
+
+ // Empty API port file, port 0 = truncate file
+ saveport(0);
+
+ //Remove PID file
+ removepid();
+
+ // Remove shared memory objects
+ // Important: This invalidated all objects such as
+ // counters-> ... etc.
+ // This should be the last action when cleaning up
+ destroy_shmem();
+
+ char buffer[42] = { 0 };
+ format_time(buffer, 0, timer_elapsed_msec(EXIT_TIMER));
+ logg("########## FTL terminated after%s (code %i)! ##########", buffer, ret);
+}

src/daemon.h

@@ -12,11 +12,10 @@
 
 void go_daemon(void);
 void savepid(void);
-char * getUserName(void);
-void removepid(void);
+char *getUserName(void);
 void delay_startup(void);
 bool is_fork(const pid_t mpid, const pid_t pid) __attribute__ ((const));
-
+void cleanup(const int ret);
 
 #include <sys/syscall.h>
 #include <unistd.h>

src/database/database-thread.c

@@ -36,7 +36,8 @@ void *DB_thread(void *val)
  // to the database
  time_t lastDBsave = time(NULL) - time(NULL)%config.DBinterval;
 
- while(!killed)
+ // Run as long as this thread is not canceled
+ while(true)
  {
  if(FTL_DB_avail())
  {

src/database/message-table.c

@@ -17,6 +17,8 @@
 #include "gravity-db.h"
 // cli_mode
 #include "../args.h"
+// cleanup()
+#include "../daemon.h"
 
 static const char *message_types[MAX_MESSAGE] =
  { "REGEX", "SUBNET", "HOSTNAME", "DNSMASQ_CONFIG" };
@@ -271,4 +273,8 @@ void logg_fatal_dnsmasq_message(const char *message)
  // Log to database (we have to open the database at this point)
  dbopen();
  add_message(DNSMASQ_CONFIG_MESSAGE, message, 0);
+
+ // FTL will dies after this point, so we should make sure to clean up
+ // behind ourselves
+ cleanup(EXIT_FAILURE);
 }

src/database/query-table.c

@@ -42,8 +42,14 @@ int get_number_of_queries_in_DB(void)
  return result;
 }
 
-void DB_save_queries(void)
+bool DB_save_queries(void)
 {
+ // The database may be unavailable, e.g. when disabled
+ if(!FTL_DB_avail())
+ {
+ return false;
+ }
+
  // Start database timer
  if(config.debug & DEBUG_DATABASE)
  timer_start(DATABASE_WRITE_TIMER);
@@ -65,7 +71,7 @@ void DB_save_queries(void)
  }
 
  logg("%s: Storing queries in long-term database failed: %s", text, sqlite3_errstr(rc));
- return;
+ return false;
  }
 
  rc = sqlite3_prepare_v2(FTL_db, "INSERT INTO queries VALUES (NULL,?,?,?,?,?,?,?)", -1, &stmt, NULL);
@@ -88,7 +94,7 @@ void DB_save_queries(void)
  logg("%s: Storing queries in long-term database failed: %s\n", text, sqlite3_errstr(rc));
  logg("%s Keeping queries in memory for later new attempt", spaces);
  saving_failed_before = true;
- return;
+ return false;
  }
 
  // Get last ID stored in the database
@@ -230,7 +236,7 @@ void DB_save_queries(void)
  else
  dbclose();
 
- return;
+ return false;
  }
 
  // Finish prepared statement
@@ -247,7 +253,7 @@ void DB_save_queries(void)
  else
  dbclose();
 
- return;
+ return false;
  }
 
  // Store index for next loop interation round and update last time stamp
@@ -268,6 +274,8 @@ void DB_save_queries(void)
  saving_failed_before = false;
  }
  }
+
+ return true;
 }
 
 void delete_old_queries_in_DB(void)

src/database/query-table.h

@@ -12,7 +12,7 @@
 
 int get_number_of_queries_in_DB(void);
 void delete_old_queries_in_DB(void);
-void DB_save_queries(void);
+bool DB_save_queries(void);
 void DB_read_queries(void);
 
 #endif //DATABASE_QUERY_TABLE_H

src/dnsmasq/dnsmasq.c

@@ -19,6 +19,8 @@
 
 #include "dnsmasq.h"
 #include "../dnsmasq_interface.h"
+// killed
+#include "../signals.h"
 
 struct daemon *daemon;
 
@@ -1026,6 +1028,10 @@ int main_dnsmasq (int argc, char **argv)
  /* Using inotify, have to select a resolv file at startup */
  poll_resolv(1, 0, now);
 #endif
+
+ /*** Pi-hole modification ***/
+ terminate = killed;
+ /****************************/
 
  while (!terminate)
  {

src/dnsmasq_interface.c

@@ -1768,10 +1768,6 @@ void FTL_fork_and_bind_sockets(struct passwd *ent_pw)
  pthread_attr_t attr;
  // Initialize thread attributes object with default attribute values
  pthread_attr_init(&attr);
- // When a detached thread terminates, its resources are automatically
- // released back to the system without the need for another thread to
- // join with the terminated thread
- pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
 
  // Start TELNET IPv4 thread
  if(pthread_create( &telnet_listenthreadv4, &attr, telnet_listening_thread_IPv4, NULL ) != 0)

src/gc.c

@@ -41,7 +41,9 @@ void *GC_thread(void *val)
  // Remember when we last ran the actions
  time_t lastGCrun = time(NULL) - time(NULL)%GCinterval;
  time_t lastRateLimitCleaner = time(NULL);
- while(!killed)
+
+ // Run as long as this thread is not canceled
+ while(true)
  {
  const time_t now = time(NULL);
  if((unsigned int)(now - lastRateLimitCleaner) >= config.rate_limit.interval)

src/log.c

@@ -18,14 +18,13 @@
 #include "main.h"
 // global variable daemonmode
 #include "args.h"
-// global counters variable
+// global counters variable and shared logfile lock
 #include "shmem.h"
 // main_pid()
 #include "signals.h"
 // logg_fatal_dnsmasq_message()
 #include "database/message-table.h"
 
-static pthread_mutex_t lock;
 static FILE *logfile = NULL;
 static bool FTL_log_ready = false;
 static bool print_log = true, print_stdout = true;
@@ -46,14 +45,6 @@ void open_FTL_log(const bool init)
 {
  if(init)
  {
- // Initialize logging mutex
- if (pthread_mutex_init(&lock, NULL) != 0)
- {
- printf("FATAL: Log mutex init failed\n");
- // Return failure
- exit(EXIT_FAILURE);
- }
-
  // Obtain log file location
  getLogFilePath();
  }
@@ -111,7 +102,7 @@ void _FTL_log(const bool newline, const char *format, ...)
  if(!print_log && !print_stdout)
  return;
 
- pthread_mutex_lock(&lock);
+ lock_log();
 
  get_timestr(timestring, time(NULL), true);
 
@@ -175,7 +166,7 @@ void _FTL_log(const bool newline, const char *format, ...)
  close_FTL_log();
  }
 
- pthread_mutex_unlock(&lock);
+ unlock_log();
 }
 
 // Log helper activity (may be script or lua)