Modify arch-tests, refine how we detect an already running pihole-FTL process and how we get other processes names from proc (the existing method didn't work on alpine:arm) and remove the obsolete struct size checking

Signed-off-by: DL6ER <dl6er@dl6er.de>

src/args.c

@@ -811,12 +811,6 @@ void parse_args(int argc, char* argv[])
 			exit(EXIT_SUCCESS);
 		}
 
-		// Return number of errors on this undocumented flag
-		if(strcmp(argv[i], "--check-structs") == 0)
-		{
-			exit(check_struct_sizes());
-		}
-
 		// Complain if invalid options have been found
 		if(!ok)
 		{

src/args.h

@@ -27,7 +27,4 @@ const char *cli_over(void) __attribute__ ((pure));
 
 void test_dnsmasq_options(int argc, const char *argv[]);
 
-// defined in dnsmasq_interface.c
-int check_struct_sizes(void);
-
 #endif //ARGS_H

src/config/password.c

@@ -12,8 +12,8 @@
 #include "log.h"
 #include "config/config.h"
 #include "password.h"
-// genrandom()
-#include <sys/random.h>
+// genrandom() with fallback
+#include "daemon.h"
 
 // Randomness generator
 #include "webserver/x509.h"

src/dnsmasq_interface.c

@@ -3398,35 +3398,6 @@ void FTL_dnsmasq_log(const char *payload, const int length)
 	unlock_shm();
 }
 
-// Check sizes of all important in-memory objects. This routine returns the number of
-// errors found (i.e., a return value of 0 is what we want and expect)
-int check_struct_sizes(void)
-{
-	int result = 0;
-	const size_t time_t_size = sizeof(time_t);
-	result += check_one_struct("struct conf_item", sizeof(struct conf_item), 72, 52);
-	result += check_one_struct("struct config", sizeof(struct config),
-	                           CONFIG_ELEMENTS*sizeof(struct conf_item),
-	                           CONFIG_ELEMENTS*sizeof(struct conf_item));
-	result += check_one_struct("queriesData", sizeof(queriesData), 72, 64);
-	result += check_one_struct("upstreamsData", sizeof(upstreamsData), 640, 628);
-	result += check_one_struct("clientsData", sizeof(clientsData), 664 + time_t_size, 648 + time_t_size);
-	result += check_one_struct("domainsData", sizeof(domainsData), 24, 20);
-	result += check_one_struct("DNSCacheData", sizeof(DNSCacheData), 24, 20);
-	result += check_one_struct("ednsData", sizeof(ednsData), 76, 76);
-	result += check_one_struct("overTimeData", sizeof(overTimeData), 24 + time_t_size, 20 + time_t_size);
-	result += check_one_struct("regexData", sizeof(regexData), 80, 52);
-	result += check_one_struct("SharedMemory", sizeof(SharedMemory), 24, 12);
-	result += check_one_struct("ShmSettings", sizeof(ShmSettings), 16, 16);
-	result += check_one_struct("countersStruct", sizeof(countersStruct), 292, 292);
-	result += check_one_struct("sqlite3_stmt_vec", sizeof(sqlite3_stmt_vec), 32, 16);
-
-	if(result == 0)
-		printf("All okay\n");
-
-	return result;
-}
-
 static const char *check_dnsmasq_name(const char *name)
 {
 	// Special domain name handling

src/procps.c

@@ -15,10 +15,20 @@
 // getpid()
 #include <unistd.h>
 #include <sys/times.h>
+// config
+#include "config/config.h"
 
 #define PROCESS_NAME   "pihole-FTL"
+#define PROC_PATH_SIZ  32
 
-static bool get_process_name(const pid_t pid, char name[16])
+// This function tries to obtain the process name of a given PID
+// It returns true on success, false otherwise and stores the process name in
+// the given buffer
+// The preferred mechanism is to use /proc/<pid>/exe, but if that fails, we try
+// to parse /proc/<pid>/comm. The latter is not guaranteed to be correct (e.g.
+// processes can easily change it themselves using prctl with PR_SET_NAME), but
+// it is better than nothing.
+static bool get_process_name(const pid_t pid, char name[PROC_PATH_SIZ])
 {
 	if(pid == 0)
 	{
@@ -27,7 +37,25 @@ static bool get_process_name(const pid_t pid, char name[16])
 	}
 
 	// Try to open comm file
-	char filename[sizeof("/proc/%u/task/%u/comm") + sizeof(int)*3 * 2];
+	char filename[sizeof("/proc/%d/exe") + sizeof(int)*3];
+	snprintf(filename, sizeof(filename), "/proc/%d/exe", pid);
+
+	// Read link destination
+	ssize_t len = readlink(filename, name, PROC_PATH_SIZ);
+	if(len > 0)
+	{
+		// If readlink() succeeded, terminate string
+		name[len] = '\0';
+
+		// Strip path from name
+		char *ptr = strrchr(name, '/');
+		if(ptr != NULL)
+			memmove(name, ptr+1, len - (ptr - name));
+
+		return true;
+	}
+
+	// If readlink() failed, try to open comm file
 	snprintf(filename, sizeof(filename), "/proc/%d/comm", pid);
 	FILE *f = fopen(filename, "r");
 	if(f == NULL)
@@ -35,13 +63,20 @@ static bool get_process_name(const pid_t pid, char name[16])
 
 	// Read name from opened file
 	if(fscanf(f, "%15s", name) != 1)
-		false;
+	{
+		fclose(f);
+		return false;
+	}
+
+	// Close file
 	fclose(f);
 
 	return true;
 }
 
-
+// This function tries to obtain the parent process ID of a given PID
+// It returns true on success, false otherwise and stores the parent PID in
+// the given pid_t pointer
 static bool get_process_ppid(const pid_t pid, pid_t *ppid)
 {
 	// Try to open status file
@@ -63,6 +98,9 @@ static bool get_process_ppid(const pid_t pid, pid_t *ppid)
 	return true;
 }
 
+// This function tries to obtain the process creation time of a given PID
+// It returns true on success, false otherwise and stores the creation time in
+// the given buffer
 static bool get_process_creation_time(const pid_t pid, char timestr[TIMESTR_SIZE])
 {
 	// Try to open comm file
@@ -76,13 +114,73 @@ static bool get_process_creation_time(const pid_t pid, char timestr[TIMESTR_SIZE
 	return true;
 }
 
+// This function prints an info message about if another FTL process is already
+// running. It returns true if another FTL process is already running, false
+// otherwise.
 bool check_running_FTL(void)
 {
 	DIR *dirPos;
 	struct dirent *entry;
 	const pid_t ourselves = getpid();
-	bool process_running = false;
+	bool already_running = false;
+	pid_t pid = 0;
 
+	// First we try to read the PID file and compare the PID in there with
+	// our own PID. If the PID file does not exist or does not contain our
+	// PID, we try to find another FTL process by looking at the process
+	// list further down.
+	if(config.files.pid.v.s != NULL)
+	{
+		FILE *pidFile = fopen(config.files.pid.v.s, "r");
+		if(pidFile != NULL)
+		{
+			if(fscanf(pidFile, "%d", &pid) == 1)
+			{
+				if(pid == ourselves)
+				{
+					log_debug(DEBUG_SHMEM, "PID file contains our own PID");
+				}
+				else
+				{
+					// Note: kill(pid, 0) does not send a
+					// signal, but merely checks if the
+					// process exists If the process does
+					// not exist, kill() returns -1 and sets
+					// errno to ESRCH. However, if the
+					// process exists, but security
+					// restrictions tell the system to deny
+					// its existence, we cannot distinguish
+					// between the process not existing and
+					// the process existing but being denied
+					// to us. In that case, our fallback
+					// solution below kicks in and iterates
+					// over /proc instead.
+					already_running = kill(pid, 0) == 0;
+					log_debug(DEBUG_SHMEM, "PID file contains PID %d (%s), we are %d",
+					          pid, already_running ? "running" : "dead", ourselves);
+				}
+			}
+			else
+			{
+				log_debug(DEBUG_SHMEM, "Failed to parse PID in PID file");
+			}
+			fclose(pidFile);
+		}
+		else
+		{
+			log_debug(DEBUG_SHMEM, "Failed to open PID file");
+		}
+	}
+
+	// If already_running is true, we are done
+	if(already_running)
+	{
+		log_info("%s is already running (PID %d)!", PROCESS_NAME, pid);
+		return true;
+	}
+
+	// If the PID file does not contain our own PID, we try to find a running
+	// process with the same name as our own process
 	// Open /proc
 	errno = 0;
 	if ((dirPos = opendir("/proc")) == NULL)
@@ -95,6 +193,7 @@ bool check_running_FTL(void)
 	// This is much more efficient than iterating over all possible PIDs
 	pid_t last_pid = 0;
 	size_t last_len = 0u;
+	log_debug(DEBUG_SHMEM, "Reading /proc/[0-9]*");
 	while ((entry = readdir(dirPos)) != NULL)
 	{
 		// We are only interested in subdirectories of /proc
@@ -105,17 +204,19 @@ bool check_running_FTL(void)
 			continue;
 
 		// Extract PID
-		const pid_t pid = strtol(entry->d_name, NULL, 10);
+		pid = strtol(entry->d_name, NULL, 10);
+
+		// Get process name
+		char name[PROC_PATH_SIZ] = { 0 };
+		if(!get_process_name(pid, name))
+			continue;
+
+		log_debug(DEBUG_SHMEM, "PID: %d -> name: %s%s", pid, name, pid == ourselves ? " (us)" : "");
 
 		// Skip our own process
 		if(pid == ourselves)
 			continue;
 
-		// Get process name
-		char name[16] = { 0 };
-		if(!get_process_name(pid, name))
-			continue;
-
 		// Only process this is this is our own process
 		if(strcasecmp(name, PROCESS_NAME) != 0)
 			continue;
@@ -124,27 +225,29 @@ bool check_running_FTL(void)
 		pid_t ppid;
 		if(!get_process_ppid(pid, &ppid))
 			continue;
-		char ppid_name[16] = { 0 };
+		char ppid_name[PROC_PATH_SIZ] = { 0 };
 		if(!get_process_name(ppid, ppid_name))
 			continue;
 
+		log_debug(DEBUG_SHMEM, " └ PPID: %d -> name: %s", ppid, ppid_name);
+
 		char timestr[TIMESTR_SIZE] = { 0 };
 		get_process_creation_time(pid, timestr);
 
 		// If this is the first process we log, add a header
-		if(!process_running)
+		if(!already_running)
 		{
-			process_running = true;
+			already_running = true;
 			log_info("%s is already running!", PROCESS_NAME);
 		}
 
 		if(last_pid != ppid)
 		{
 			// Independent process, may be child of init/systemd
-			log_info("%s (%d) ──> %s (PID %d, started %s)",
+			log_info("%s (PID %d) ──> %s (PID %d, started %s)",
 			         ppid_name, ppid, name, pid, timestr);
 			last_pid = pid;
-			last_len = snprintf(NULL, 0, "%s (%d) ──> ", ppid_name, ppid);
+			last_len = snprintf(NULL, 0, "%s (PID %d) ──> ", ppid_name, ppid);
 		}
 		else
 		{
@@ -154,9 +257,10 @@ bool check_running_FTL(void)
 			         (int)last_len, "", name, pid, timestr);
 		}
 	}
+	log_debug(DEBUG_SHMEM, "Done reading /proc/[0-9]*");
 
 	closedir(dirPos);
-	return process_running;
+	return already_running;
 }
 
 bool read_self_memory_status(struct statm_t *result)

test/arch_test.sh

@@ -9,13 +9,16 @@
 # This file is copyright under the latest version of the EUPL.
 # Please see LICENSE file for your rights under this license. */
 
+okay=true
+
 check_libs() {
   mapfile -t libs < <(readelf -d ./pihole-FTL | grep "Shared library" | grep -oE "\[.*\]")
   if [[ "${libs[*]}" != "${1}" ]]; then
     echo "Wrong libraries"
-    echo "Found: ${libs[*]}"
-    echo "Expected: ${1}"
-    exit 1
+    echo "   Expected: ${1}"
+    echo "   Found: ${libs[*]}"
+    okay=false
+    return
   fi
   echo "Library checks: OK (using ${#libs[*]} shared libraries)"
 }
@@ -24,9 +27,10 @@ check_machine() {
   mapfile -t header < <(readelf -h ./pihole-FTL | grep -E "(Class)|(Machine)" | sed "s/.*://;s/ \{2,\}//g;")
   if [[ "${header[0]}" != "${1}" || "${header[1]}" != "${2}" ]]; then
     echo "Wrong machine"
-    echo "Expected: Class: ${1} Machine: ${2}"
-    echo "Found: Class: ${header[0]} Machine: ${header[1]}"
-    exit 1
+    echo "   Expected: Class: ${1} Machine: ${2}"
+    echo "   Found: Class: ${header[0]} Machine: ${header[1]}"
+    okay=false
+    return
   fi
   echo "Machine checks: OK (${1} binary for ${2})"
 }
@@ -35,9 +39,10 @@ check_CPU_arch() {
   cpuarch="$(readelf -A ./pihole-FTL | grep "Tag_CPU_arch:" | sed "s/^ *//")"
   if [[ "${cpuarch}" != "Tag_CPU_arch: ${1}" ]]; then
     echo "Wrong CPU arch"
-    echo "Expected: Tag_CPU_arch: ${1}"
-    echo "Found: ${cpuarch}"
-    exit 1
+    echo "   Expected: Tag_CPU_arch: ${1}"
+    echo "   Found: ${cpuarch}"
+    okay=false
+    return
   fi
   echo "CPU architecture checks: OK (${1})"
 }
@@ -46,9 +51,10 @@ check_FP_arch() {
   fparch="$(readelf -A ./pihole-FTL | grep "Tag_FP_arch:" | sed "s/^ *//")"
   if [[ "${fparch}" != "Tag_FP_arch: ${1}" && -n "${1}" ]]; then
     echo "Wrong FP arch"
-    echo "Expected: Tag_FP_arch: ${1}"
-    echo "Found: ${fparch}"
-    exit 1
+    echo "   Expected: Tag_FP_arch: ${1}"
+    echo "   Found: ${fparch}"
+    okay=false
+    return
   fi
   echo "FP architecture checks: OK (${1})"
 }
@@ -57,9 +63,10 @@ check_file() {
   filedetails="$(file -b pihole-FTL | sed "s/, BuildID[^,]*//g")"
   if [[ "${filedetails}" != "${1}" ]]; then
     echo "Wrong binary classification"
-    echo "Expected: ${1}"
-    echo "Found: ${filedetails}"
-    exit 1
+    echo "   Expected: ${1}"
+    echo "   Found: ${filedetails}"
+    okay=false
+    return
   fi
   echo "Binary classification checks: OK (${1})"
 }
@@ -68,7 +75,8 @@ check_static() {
   if readelf -l ./pihole-FTL | grep -q INTERP; then
     echo "Not a static executable, depends on dynamic interpreter"
     ldd ./pihole-FTL
-    exit 1
+    okay=false
+    return
   fi
   echo "Static executable check: OK"
 }
@@ -87,12 +95,14 @@ elif [[ "${CI_ARCH}" == "linux/386" ]]; then
   check_libs "" # No dependency on any shared library is intended
   check_file "ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV), statically linked, with debug_info, not stripped"
 
-elif [[ "${CI_ARCH}" == "linux/arm64/v8" || "${CI_ARCH}" == "linux/arm64" ]]; then
+elif [[ "${CI_ARCH}" == "linux/arm/v5" ]]; then
 
-  check_machine "ELF64" "AArch64"
-  check_static # Binary should not rely on any dynamic interpreter
-  check_libs "" # No dependency on any shared library is intended
-  check_file "ELF 64-bit LSB shared object, ARM aarch64, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, for GNU/Linux 3.7.0, with debug_info, not stripped"
+  check_machine "ELF32" "ARM"
+  check_libs "[libm.so.6] [librt.so.1] [libgcc_s.so.1] [libpthread.so.0] [libc.so.6] [ld-linux.so.3]"
+  check_file "ELF 32-bit LSB pie executable, ARM, EABI5 version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux.so.3, for GNU/Linux 3.2.0, with debug_info, not stripped"
+
+  check_CPU_arch "v5TE"
+  check_FP_arch ""
 
 elif [[ "${CI_ARCH}" == "linux/arm/v6" ]]; then
 
@@ -101,8 +111,9 @@ elif [[ "${CI_ARCH}" == "linux/arm/v6" ]]; then
   check_libs "" # No dependency on any shared library is intended
   check_file "ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), statically linked, with debug_info, not stripped"
 
-  check_CPU_arch "v6"
-  check_FP_arch "VFPv2"
+  check_CPU_arch "v6KZ"
+  # VFPv3 is backwards compatible with VFPv2
+  check_FP_arch "VFPv3"
 
 elif [[ "${CI_ARCH}" == "linux/arm/v7" ]]; then
 
@@ -112,7 +123,14 @@ elif [[ "${CI_ARCH}" == "linux/arm/v7" ]]; then
   check_file "ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), statically linked, with debug_info, not stripped"
 
   check_CPU_arch "v7"
-  check_FP_arch "VFPv3-D16"
+  check_FP_arch "VFPv3"
+
+elif [[ "${CI_ARCH}" == "linux/arm64/v8" || "${CI_ARCH}" == "linux/arm64" ]]; then
+
+  check_machine "ELF64" "AArch64"
+  check_static # Binary should not rely on any dynamic interpreter
+  check_libs "" # No dependency on any shared library is intended
+  check_file "ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, with debug_info, not stripped"
 
 elif [[ "${CI_ARCH}" == "linux/riscv64" ]]; then
 
@@ -123,9 +141,14 @@ elif [[ "${CI_ARCH}" == "linux/riscv64" ]]; then
 
 else
 
-  echo "Invalid job ${CI_ARCH}"
+  echo "Unknown architecture '${CI_ARCH}'"
   exit 1
 
 fi
 
+if [[ "${okay}" == "false" ]]; then
+  echo "Binary checks failed"
+  exit 1
+fi
+
 exit 0

test/run.sh

@@ -1,7 +1,7 @@
 #!/bin/bash
 
-# Only run tests on x86_* targets (where the CI can natively run the binaries)
-if [[ ${CI} == "true" && "${CI_ARCH}" != "x86_"* ]]; then
+# Skip tests on targets not supporting them
+if [[ ${TEST} == "false" ]]; then
   echo "Skipping tests (CI_ARCH: ${CI_ARCH})!"
   exit 0
 fi

test/test_suite.bats

@@ -29,7 +29,7 @@
   run bash -c 'su pihole -s /bin/sh -c "/home/pihole/pihole-FTL -f"'
   printf "%s\n" "${lines[@]}"
   [[ "${lines[@]}" == *"CRIT: Initialization of shared memory failed."* ]]
-  [[ "${lines[@]}" == *"INFO: pihole-FTL is already running!"* ]]
+  [[ "${lines[@]}" == *"INFO: pihole-FTL is already running"* ]]
 }
 
 @test "dnsmasq options as expected" {
@@ -969,12 +969,6 @@
   [[ ${lines[0]} == *"using ${compiler_version}"* ]]
 }
 
-@test "Struct sizes are as expected" {
-  run bash -c './pihole-FTL --check-structs'
-  printf "%s\n" "${lines[@]}"
-  [[ $status == 0 ]]
-}
-
 @test "No errors on setting busy handlers for the databases" {
   run bash -c 'grep -c "Cannot set busy handler" /var/log/pihole/FTL.log'
   printf "%s\n" "${lines[@]}"

tools/socket_client.c

@@ -1,85 +0,0 @@
-/* Pi-hole: A black hole for Internet advertisements
-*  (c) 2017 Pi-hole, LLC (https://pi-hole.net)
-*  Network-wide ad blocking via your own hardware.
-*
-*  FTL Engine
-*  Unix socket connection test program
-*
-*  This file is copyright under the latest version of the EUPL.
-*  Please see LICENSE file for your rights under this license. */
-#include <sys/types.h>
-#include <sys/socket.h>
-#include <sys/un.h>
-#include <netinet/in.h>
-#include <arpa/inet.h>
-#include <unistd.h>
-#include <stdlib.h>
-#include <stdio.h>
-#include <string.h>
-#include <errno.h>
-
-#define BUF 1024
-
-int main (int argc, char **argv) {
-	int socketfd;
-	char *buffer = malloc (BUF);
-	struct sockaddr_un address;
-	ssize_t size;
-	int ret;
-
-	// Create socket
-	socketfd = socket(PF_LOCAL, SOCK_STREAM, 0);
-	if(socketfd <= 0)
-	{
-		printf("Error creating socket!\n");
-		exit(EXIT_FAILURE);
-	}
-	printf("Socket created\n");
-
-	// Set socket family to local socket (not an Internet socket)
-	address.sun_family = AF_LOCAL;
-
-	char *command = ">stats";
-	strcpy(address.sun_path,"/run/pihole/FTL.sock");
-
-	int i;
-	for(i = 1; i < argc; i++) {
-		// Get command
-		if(strstr(argv[i], ">") == argv[i]) {
-			command = argv[i];
-			continue;
-		}
-	}
-
-	// Connect to the socket provided by pihole-FTL
-	ret = connect(socketfd, (struct sockaddr *) &address, sizeof (address));
-	if (ret != 0)
-	{
-		printf("Error establishing connection! %s\n", strerror(errno));
-		exit(EXIT_FAILURE);
-	}
-	printf("Connection established\n");
-
-	// As an example, we query the current statistics from FTL through the socket here
-	sprintf(buffer, command);
-	send(socketfd, buffer, strlen (buffer), 0);
-
-	// Try to receive data until either recv() fails or we see "--EOM--"
-	while((size = recv(socketfd, buffer, BUF-1, 0)) > -1)
-	{
-		// Print received data to stdout
-		for(i = 0; i < size; ++i) {
-			printf("%02x ", (unsigned char) buffer[i]);
-		}
-
-		// Exit on End Of Message
-		if((unsigned char) buffer[size-1] == 0xc1)
-			break;
-	}
-
-	printf("\n");
-
-	// Close Unix socket connection
-	close(socketfd);
-	return EXIT_SUCCESS;
-}