Merge pull request #1172 from pi-hole/tweak/static-code-analysis

Improvements based on static-analysis of source code

src/api/api.c

@@ -711,7 +711,9 @@ void getAllQueries(const char *client_message, const int *sock)
 	if(command(client_message, ">getallqueries-forward")) {
 		// Get forward destination name we want to see only (limit length to 255 chars)
 		forwarddest = calloc(256, sizeof(char));
-		if(forwarddest == NULL) return;
+		if(forwarddest == NULL)
+			return;
+
 		sscanf(client_message, ">getallqueries-forward %255s", forwarddest);
 		filterforwarddest = true;
 
@@ -763,7 +765,12 @@ void getAllQueries(const char *client_message, const int *sock)
 	if(command(client_message, ">getallqueries-domain")) {
 		// Get domain name we want to see only (limit length to 255 chars)
 		domainname = calloc(256, sizeof(char));
-		if(domainname == NULL) return;
+		if(domainname == NULL)
+		{
+			if(forwarddest) free(forwarddest);
+			return;
+		}
+
 		sscanf(client_message, ">getallqueries-domain %255s", domainname);
 		filterdomainname = true;
 		// Iterate through all known domains
@@ -786,6 +793,7 @@ void getAllQueries(const char *client_message, const int *sock)
 			// Requested domain has not been found, we directly
 			// exit here as there is no data to be returned
 			free(domainname);
+			if(forwarddest) free(forwarddest);
 			return;
 		}
 	}
@@ -794,7 +802,13 @@ void getAllQueries(const char *client_message, const int *sock)
 	if(command(client_message, ">getallqueries-client")) {
 		// Get client name we want to see only (limit length to 255 chars)
 		clientname = calloc(256, sizeof(char));
-		if(clientname == NULL) return;
+		if(clientname == NULL)
+		{
+			if(forwarddest) free(forwarddest);
+			if(domainname) free(domainname);
+			return;
+		}
+
 		if(command(client_message, ">getallqueries-client-blocked"))
 		{
 			showpermitted = false;
@@ -834,6 +848,8 @@ void getAllQueries(const char *client_message, const int *sock)
 			// Requested client has not been found, we directly
 			// exit here as there is no data to be returned
 			free(clientname);
+			if(forwarddest) free(forwarddest);
+			if(domainname) free(domainname);
 			return;
 		}
 
@@ -1045,11 +1061,11 @@ void getAllQueries(const char *client_message, const int *sock)
 
 			// Use a fixstr because the length of qtype is always 4 (max is 31 for fixstr)
 			if(!pack_fixstr(*sock, qtype))
-				return;
+				break;
 
 			// Use str32 for domain and client because we have no idea how long they will be (max is 4294967295 for str32)
 			if(!pack_str32(*sock, domain) || !pack_str32(*sock, clientIPName))
-				return;
+				break;
 
 			pack_uint8(*sock, query->status);
 			pack_uint8(*sock, query->dnssec);
@@ -1174,8 +1190,7 @@ void getDBstats(const int *sock)
 	// Get file details
 	unsigned long long int filesize = get_FTL_db_filesize();
 
-	char *prefix = calloc(2, sizeof(char));
-	if(prefix == NULL) return;
+	char prefix[2] = { 0 };
 	double formated = 0.0;
 	format_memory_size(prefix, filesize, &formated);
 

src/api/socket.c

@@ -77,7 +77,8 @@ static bool bind_to_telnet_port_IPv4(int *socketdescriptor)
 	// new instance will fail if there were connections open to the previous
 	// instance when you killed it. Those connections will hold the TCP port in
 	// the TIME_WAIT state for 30-120 seconds, so you fall into case 1 above.
-	setsockopt(*socketdescriptor, SOL_SOCKET, SO_REUSEADDR, &(int){ 1 }, sizeof(int));
+	if(setsockopt(*socketdescriptor, SOL_SOCKET, SO_REUSEADDR, &(int){ 1 }, sizeof(int)) != 0)
+		logg("WARN: allowing re-binding (IPv6) failed: %s", strerror(errno));
 
 	struct sockaddr_in serv_addr4;
 	// set all values in the buffer to zero
@@ -123,7 +124,8 @@ static bool bind_to_telnet_port_IPv6(int *socketdescriptor)
 	// stricted  to  sending  and receiving IPv6 packets only.  In this
 	// case, an IPv4 and an IPv6 application can bind to a single  port
 	// at the same time.
-	setsockopt(*socketdescriptor, IPPROTO_IPV6, IPV6_V6ONLY, &(int){ 1 }, sizeof(int));
+	if(setsockopt(*socketdescriptor, IPPROTO_IPV6, IPV6_V6ONLY, &(int){ 1 }, sizeof(int)) != 0)
+		logg("WARN: setting socket to IPv6-only failed: %s", strerror(errno));
 
 	// Set SO_REUSEADDR to allow re-binding to the port that has been used
 	// previously by FTL. A common pattern is that you change FTL's
@@ -132,7 +134,8 @@ static bool bind_to_telnet_port_IPv6(int *socketdescriptor)
 	// new instance will fail if there were connections open to the previous
 	// instance when you killed it. Those connections will hold the TCP port in
 	// the TIME_WAIT state for 30-120 seconds, so you fall into case 1 above.
-	setsockopt(*socketdescriptor, SOL_SOCKET, SO_REUSEADDR, &(int){ 1 }, sizeof(int));
+	if(setsockopt(*socketdescriptor, SOL_SOCKET, SO_REUSEADDR, &(int){ 1 }, sizeof(int)) != 0)
+		logg("WARN: allowing re-binding (IPv6) failed: %s", strerror(errno));
 
 	struct sockaddr_in6 serv_addr;
 	// set all values in the buffer to zero
@@ -341,8 +344,10 @@ static void *telnet_connection_handler_thread(void *socket_desc)
 	ssize_t n;
 	while((n = recv(sock,client_message,SOCKETBUFFERLEN-1, 0)))
 	{
-		if (n > 0)
+		if (n > 0 && n < SOCKETBUFFERLEN)
 		{
+			// Null-terminate client string
+			client_message[n] = '\0';
 			char *message = strdup(client_message);
 			if(message == NULL) break;
 
@@ -419,8 +424,10 @@ static void *socket_connection_handler_thread(void *socket_desc)
 	ssize_t n;
 	while((n = recv(sock,client_message,SOCKETBUFFERLEN-1, 0)))
 	{
-		if (n > 0)
+		if (n > 0 && n < SOCKETBUFFERLEN)
 		{
+			// Null-terminate client string
+			client_message[n] = '\0';
 			char *message = strdup(client_message);
 			if(message == NULL) break;
 

src/capabilities.c

@@ -17,9 +17,9 @@
 #include "config.h"
 #include "log.h"
 
-static const unsigned int capabilityIDs[]   = { CAP_CHOWN ,  CAP_DAC_OVERRIDE ,  CAP_DAC_READ_SEARCH ,  CAP_FOWNER ,  CAP_FSETID ,  CAP_KILL ,  CAP_SETGID ,  CAP_SETUID ,  CAP_SETPCAP ,  CAP_LINUX_IMMUTABLE ,  CAP_NET_BIND_SERVICE ,  CAP_NET_BROADCAST ,  CAP_NET_ADMIN ,  CAP_NET_RAW ,  CAP_IPC_LOCK ,  CAP_IPC_OWNER ,  CAP_SYS_MODULE ,  CAP_SYS_RAWIO ,  CAP_SYS_CHROOT ,  CAP_SYS_PTRACE ,  CAP_SYS_PACCT ,  CAP_SYS_ADMIN ,  CAP_SYS_BOOT ,  CAP_SYS_NICE ,  CAP_SYS_RESOURCE ,  CAP_SYS_TIME ,  CAP_SYS_TTY_CONFIG ,  CAP_MKNOD ,  CAP_LEASE ,  CAP_AUDIT_WRITE ,  CAP_AUDIT_CONTROL ,  CAP_SETFCAP ,  CAP_MAC_OVERRIDE ,  CAP_MAC_ADMIN ,  CAP_SYSLOG ,  CAP_WAKE_ALARM ,  CAP_BLOCK_SUSPEND ,  CAP_AUDIT_READ };
-static const char*        capabilityNames[] = {"CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_DAC_READ_SEARCH", "CAP_FOWNER", "CAP_FSETID", "CAP_KILL", "CAP_SETGID", "CAP_SETUID", "CAP_SETPCAP", "CAP_LINUX_IMMUTABLE", "CAP_NET_BIND_SERVICE", "CAP_NET_BROADCAST", "CAP_NET_ADMIN", "CAP_NET_RAW", "CAP_IPC_LOCK", "CAP_IPC_OWNER", "CAP_SYS_MODULE", "CAP_SYS_RAWIO", "CAP_SYS_CHROOT", "CAP_SYS_PTRACE", "CAP_SYS_PACCT", "CAP_SYS_ADMIN", "CAP_SYS_BOOT", "CAP_SYS_NICE", "CAP_SYS_RESOURCE", "CAP_SYS_TIME", "CAP_SYS_TTY_CONFIG", "CAP_MKNOD", "CAP_LEASE", "CAP_AUDIT_WRITE", "CAP_AUDIT_CONTROL", "CAP_SETFCAP", "CAP_MAC_OVERRIDE", "CAP_MAC_ADMIN", "CAP_SYSLOG", "CAP_WAKE_ALARM", "CAP_BLOCK_SUSPEND", "CAP_AUDIT_READ"};
-static const unsigned int numCaps = sizeof(capabilityIDs) / sizeof(const unsigned int);
+static const unsigned int capabilityIDs[]   = { CAP_CHOWN ,  CAP_DAC_OVERRIDE ,  CAP_DAC_READ_SEARCH ,  CAP_FOWNER ,  CAP_FSETID ,  CAP_KILL ,  CAP_SETGID ,  CAP_SETUID ,  CAP_SETPCAP ,  CAP_LINUX_IMMUTABLE ,  CAP_NET_BIND_SERVICE ,  CAP_NET_BROADCAST ,  CAP_NET_ADMIN ,  CAP_NET_RAW ,  CAP_IPC_LOCK ,  CAP_IPC_OWNER ,  CAP_SYS_MODULE ,  CAP_SYS_RAWIO ,  CAP_SYS_CHROOT ,  CAP_SYS_PTRACE ,  CAP_SYS_PACCT ,  CAP_SYS_ADMIN ,  CAP_SYS_BOOT ,  CAP_SYS_NICE ,  CAP_SYS_RESOURCE ,  CAP_SYS_TIME ,  CAP_SYS_TTY_CONFIG ,  CAP_MKNOD ,  CAP_LEASE ,  CAP_AUDIT_WRITE ,  CAP_AUDIT_CONTROL ,  CAP_SETFCAP };
+static const char*        capabilityNames[] = {"CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_DAC_READ_SEARCH", "CAP_FOWNER", "CAP_FSETID", "CAP_KILL", "CAP_SETGID", "CAP_SETUID", "CAP_SETPCAP", "CAP_LINUX_IMMUTABLE", "CAP_NET_BIND_SERVICE", "CAP_NET_BROADCAST", "CAP_NET_ADMIN", "CAP_NET_RAW", "CAP_IPC_LOCK", "CAP_IPC_OWNER", "CAP_SYS_MODULE", "CAP_SYS_RAWIO", "CAP_SYS_CHROOT", "CAP_SYS_PTRACE", "CAP_SYS_PACCT", "CAP_SYS_ADMIN", "CAP_SYS_BOOT", "CAP_SYS_NICE", "CAP_SYS_RESOURCE", "CAP_SYS_TIME", "CAP_SYS_TTY_CONFIG", "CAP_MKNOD", "CAP_LEASE", "CAP_AUDIT_WRITE", "CAP_AUDIT_CONTROL", "CAP_SETFCAP"};
+static const unsigned int numCaps = sizeof(capabilityIDs) / sizeof(*capabilityIDs);
 
 bool check_capabilities(void)
 {
@@ -59,12 +59,6 @@ bool check_capabilities(void)
 		for(unsigned int i = 0u; i < numCaps; i++)
 		{
 			const unsigned int capid = capabilityIDs[i];
-
-			// Check if capability is valid for the current kernel
-			// If not, exit loop early
-			if(!cap_valid(capid))
-				break;
-
 			logg("* %-24s (%02u) = %s%s%s *",
 			     capabilityNames[capid], capid,
 			     ((data->permitted   & (1 << capid)) ? "P":"-"),

src/daemon.c

@@ -127,7 +127,7 @@ static void removepid(void)
 
 char *getUserName(void)
 {
-	char * name;
+	char *name;
 	// the getpwuid() function shall search the user database for an entry with a matching uid
 	// the geteuid() function shall return the effective user ID of the calling process - this is used as the search criteria for the getpwuid() function
 	const uid_t euid = geteuid();

src/database/common.c

@@ -73,7 +73,7 @@ void _dbclose(sqlite3 **db, const char *func, const int line, const char *file)
 	}
 
 	// Always set database pointer to NULL, even when closing failed
-	*db = NULL;
+	if(db) *db = NULL;
 }
 
 sqlite3* _dbopen(bool create, const char *func, const int line, const char *file)
@@ -172,16 +172,32 @@ static bool create_counter_table(sqlite3* db)
 	SQL_bool(db, "CREATE TABLE counters ( id INTEGER PRIMARY KEY NOT NULL, value INTEGER NOT NULL );");
 
 	// ID 0 = total queries
-	db_set_counter(db, DB_TOTALQUERIES, 0);
+	if(!db_set_counter(db, DB_TOTALQUERIES, 0))
+	{
+		logg("create_counter_table(): Failed to set total queries counter to zero!");
+		return false;
+	}
 
 	// ID 1 = total blocked queries
-	db_set_counter(db, DB_BLOCKEDQUERIES, 0);
+	if(!db_set_counter(db, DB_BLOCKEDQUERIES, 0))
+	{
+		logg("create_counter_table(): Failed to set blocked queries counter to zero!");
+		return false;
+	}
 
 	// Time stamp of creation of the counters database
-	db_set_FTL_property(db, DB_FIRSTCOUNTERTIMESTAMP, (unsigned long)time(0));
+	if(!db_set_FTL_property(db, DB_FIRSTCOUNTERTIMESTAMP, (unsigned long)time(0)))
+	{
+		logg("create_counter_table(): Failed to update first counter timestamp!");
+		return false;
+	}
 
 	// Update database version to 2
-	db_set_FTL_property(db, DB_VERSION, 2);
+	if(!db_set_FTL_property(db, DB_VERSION, 2))
+	{
+		logg("create_counter_table(): Failed to update database version!");
+		return false;
+	}
 
 	return true;
 }

src/database/gravity-db.c

@@ -427,6 +427,7 @@ static bool get_client_groupids(clientsData* client)
 		{
 			logg("get_client_groupids(%s) - SQL error prepare: %s",
 				querystr, sqlite3_errstr(rc));
+			free(hwaddr); // hwaddr != NULL -> memory has been allocated
 			return false;
 		}
 
@@ -437,6 +438,7 @@ static bool get_client_groupids(clientsData* client)
 				ip, hwaddr, sqlite3_errstr(rc));
 			sqlite3_reset(table_stmt);
 			sqlite3_finalize(table_stmt);
+			free(hwaddr); // hwaddr != NULL -> memory has been allocated
 			return false;
 		}
 
@@ -462,6 +464,7 @@ static bool get_client_groupids(clientsData* client)
 			logg("get_client_groupids(\"%s\", \"%s\") - SQL error step: %s",
 				ip, hwaddr, sqlite3_errstr(rc));
 			gravityDB_finalizeTable();
+			free(hwaddr); // hwaddr != NULL -> memory has been allocated
 			return false;
 		}
 
@@ -512,6 +515,8 @@ static bool get_client_groupids(clientsData* client)
 		{
 			logg("get_client_groupids(%s) - SQL error prepare: %s",
 				querystr, sqlite3_errstr(rc));
+			if(hwaddr) free(hwaddr);
+			free(hostname); // hostname != NULL -> memory has been allocated
 			return false;
 		}
 
@@ -522,6 +527,8 @@ static bool get_client_groupids(clientsData* client)
 				ip, hostname, sqlite3_errstr(rc));
 			sqlite3_reset(table_stmt);
 			sqlite3_finalize(table_stmt);
+			if(hwaddr) free(hwaddr);
+			free(hostname); // hostname != NULL -> memory has been allocated
 			return false;
 		}
 
@@ -547,6 +554,8 @@ static bool get_client_groupids(clientsData* client)
 			logg("get_client_groupids(\"%s\", \"%s\") - SQL error step: %s",
 				ip, hostname, sqlite3_errstr(rc));
 			gravityDB_finalizeTable();
+			if(hwaddr) free(hwaddr);
+			free(hostname); // hostname != NULL -> memory has been allocated
 			return false;
 		}
 
@@ -598,6 +607,9 @@ static bool get_client_groupids(clientsData* client)
 		{
 			logg("get_client_groupids(%s) - SQL error prepare: %s",
 				querystr, sqlite3_errstr(rc));
+			if(hwaddr) free(hwaddr);
+			if(hostname) free(hostname);
+			free(interface); // interface != NULL -> memory has been allocated
 			return false;
 		}
 
@@ -608,6 +620,9 @@ static bool get_client_groupids(clientsData* client)
 				ip, interface, sqlite3_errstr(rc));
 			sqlite3_reset(table_stmt);
 			sqlite3_finalize(table_stmt);
+			if(hwaddr) free(hwaddr);
+			if(hostname) free(hostname);
+			free(interface); // interface != NULL -> memory has been allocated
 			return false;
 		}
 
@@ -633,6 +648,9 @@ static bool get_client_groupids(clientsData* client)
 			logg("get_client_groupids(\"%s\", \"%s\") - SQL error step: %s",
 				ip, interface, sqlite3_errstr(rc));
 			gravityDB_finalizeTable();
+			if(hwaddr) free(hwaddr);
+			if(hostname) free(hostname);
+			free(interface); // interface != NULL -> memory has been allocated
 			return false;
 		}