Merge pull request #1032 from pi-hole/fix/querylog_filtering

Fix for Query Log filtering and memory optimizations

src/CMakeLists.txt

@@ -143,6 +143,7 @@ set(sources
         shmem.h
         signals.c
         signals.h
+        static_assert.h
         timers.c
         timers.h
         vector.c

src/api/api.c

@@ -397,7 +397,7 @@ void getTopClients(const char *client_message, const int *sock)
 		// Get client pointer
 		const clientsData* client = getClient(clientID, true);
 		// Skip invalid clients and also those managed by alias clients
-		if(client == NULL || (!client->aliasclient && client->aliasclient_id >= 0))
+		if(client == NULL || (!client->flags.aliasclient && client->aliasclient_id >= 0))
 		{
 			temparray[clientID][0] = -1;
 			continue;
@@ -823,7 +823,7 @@ void getAllQueries(const char *client_message, const int *sock)
 				clientid = i;
 
 				// Is this a alias-client?
-				if(client->aliasclient)
+				if(client->flags.aliasclient)
 					clientid_list = get_aliasclient_list(i);
 
 				break;
@@ -891,19 +891,12 @@ void getAllQueries(const char *client_message, const int *sock)
 		if(query->status == QUERY_UNKNOWN && !(showpermitted && showblocked))
 			continue;
 
-		// 1 = gravity.list, 4 = wildcard, 5 = black.list
-		if((query->status == QUERY_GRAVITY ||
-		    query->status == QUERY_REGEX ||
-		    query->status == QUERY_BLACKLIST ||
-		    query->status == QUERY_GRAVITY_CNAME ||
-		    query->status == QUERY_REGEX_CNAME ||
-		    query->status == QUERY_BLACKLIST_CNAME) && !showblocked)
+		// Skip blocked queries when asked to
+		if(query->flags.blocked && !showblocked)
 			continue;
-		// 2 = forwarded, 3 = cached
-		if((query->status == QUERY_FORWARDED ||
-		    query->status == QUERY_CACHE ||
-		    query->status == QUERY_RETRIED ||
-		    query->status == QUERY_RETRIED_DNSSEC) && !showpermitted)
+
+		// Skip permitted queries when asked to
+		if(!query->flags.blocked && !showpermitted)
 			continue;
 
 		// Skip those entries which so not meet the requested timeframe
@@ -921,10 +914,7 @@ void getAllQueries(const char *client_message, const int *sock)
 			// If the domain of this query did not match, the CNAME
 			// domain may still match - we have to check it in
 			// addition if this query is of CNAME blocked type
-			else if((query->status == QUERY_GRAVITY_CNAME ||
-				query->status == QUERY_BLACKLIST_CNAME ||
-				query->status == QUERY_REGEX_CNAME) &&
-				query->CNAME_domainID == domainid)
+			else if(query->CNAME_domainID > -1)
 			{
 				// Get this query
 			}
@@ -959,13 +949,8 @@ void getAllQueries(const char *client_message, const int *sock)
 
 		if(filterforwarddest)
 		{
-			// Does the user want to see queries answered from blocking lists?
-			if(forwarddestid == -2 && query->status != QUERY_GRAVITY
-			                       && query->status != QUERY_REGEX
-			                       && query->status != QUERY_BLACKLIST
-			                       && query->status != QUERY_GRAVITY_CNAME
-			                       && query->status != QUERY_REGEX_CNAME
-			                       && query->status != QUERY_BLACKLIST_CNAME)
+			// Skip if not from the virtual blocking "upstream" server
+			if(forwarddestid == -2 && !query->flags.blocked)
 				continue;
 			// Does the user want to see queries answered from local cache?
 			else if(forwarddestid == -1 && query->status != QUERY_CACHE)
@@ -1017,7 +1002,7 @@ void getAllQueries(const char *client_message, const int *sock)
 		// Get IP of upstream destination, if applicable
 		in_port_t upstream_port = 0;
 		const char *upstream_name = "N/A";
-		if(query->status == QUERY_FORWARDED)
+		if(query->upstreamID > -1)
 		{
 			const upstreamsData *upstream = getUpstream(query->upstreamID, true);
 			if(upstream != NULL)
@@ -1104,15 +1089,8 @@ void getRecentBlocked(const char *client_message, const int *sock)
 		if(query == NULL)
 			continue;
 
-		if(query->status == QUERY_GRAVITY ||
-		   query->status == QUERY_REGEX ||
-		   query->status == QUERY_BLACKLIST ||
-		   query->status == QUERY_GRAVITY_CNAME ||
-		   query->status == QUERY_REGEX_CNAME ||
-		   query->status == QUERY_BLACKLIST_CNAME)
+		if(query->flags.blocked)
 		{
-			found++;
-
 			// Ask subroutine for domain. It may return "hidden" depending on
 			// the privacy settings at the time the query was made
 			const char *domain = getDomainString(query);
@@ -1123,6 +1101,9 @@ void getRecentBlocked(const char *client_message, const int *sock)
 				ssend(*sock,"%s\n", domain);
 			else if(!pack_str32(*sock, domain))
 				return;
+
+			// Only count when sent succesfully
+			found++;
 		}
 
 		if(found >= num)
@@ -1312,7 +1293,7 @@ void getClientsOverTime(const int *sock)
 			// Check if this client should be skipped
 			if(insetupVarsArray(getstr(client->ippos)) ||
 			   insetupVarsArray(getstr(client->namepos)) ||
-			   (!client->aliasclient && client->aliasclient_id > -1))
+			   (!client->flags.aliasclient && client->aliasclient_id > -1))
 				skipclient[clientID] = true;
 		}
 	}
@@ -1387,7 +1368,7 @@ void getClientNames(const int *sock)
 			// Check if this client should be skipped
 			if(insetupVarsArray(getstr(client->ippos)) ||
 			   insetupVarsArray(getstr(client->namepos)) ||
-			   (!client->aliasclient && client->aliasclient_id > -1))
+			   (!client->flags.aliasclient && client->aliasclient_id > -1))
 				skipclient[clientID] = true;
 		}
 	}
@@ -1434,7 +1415,7 @@ void getUnknownQueries(const int *sock)
 		const queriesData* query = getQuery(queryID, true);
 
 		if(query == NULL ||
-		  (query->status != QUERY_UNKNOWN && query->complete))
+		  (query->status != QUERY_UNKNOWN && query->flags.complete))
 			continue;
 
 		char type[5];
@@ -1459,7 +1440,7 @@ void getUnknownQueries(const int *sock)
 		const char *clientIP = getstr(client->ippos);
 
 		if(istelnet[*sock])
-			ssend(*sock, "%lli %i %i %s %s %s %i %s\n", (long long)query->timestamp, queryID, query->id, type, getstr(domain->domainpos), clientIP, query->status, query->complete ? "true" : "false");
+			ssend(*sock, "%lli %i %i %s %s %s %i %s\n", (long long)query->timestamp, queryID, query->id, type, getstr(domain->domainpos), clientIP, query->status, query->flags.complete ? "true" : "false");
 		else {
 			pack_int32(*sock, (int32_t)query->timestamp);
 			pack_int32(*sock, query->id);
@@ -1473,7 +1454,7 @@ void getUnknownQueries(const int *sock)
 				return;
 
 			pack_uint8(*sock, query->status);
-			pack_bool(*sock, query->complete);
+			pack_bool(*sock, query->flags.complete);
 		}
 	}
 }

src/config.h

@@ -17,24 +17,20 @@
 #include <sys/types.h>
 // typedef uni32_t
 #include <idn-int.h>
+// assert_sizeof
+#include "static_assert.h"
 
 void getLogFilePath(void);
 void read_FTLconf(void);
 void get_privacy_level(FILE *fp);
 void get_blocking_mode(FILE *fp);
 void read_debuging_settings(FILE *fp);
 
+// We do not use bitfields in here as this struct exists only once in memory.
+// Accessing bitfields may produce slightly more inefficient code on some
+// architectures (such as ARM) and savng a few bit of RAM but bloating up the
+// rest of the application each time these fields are accessed is bad.
 typedef struct {
-	int maxDBdays;
-	int port;
-	int maxlogage;
-	int dns_port;
-	unsigned int delay_startup;
-	enum debug_flags debug;
-	unsigned int network_expire;
-	enum privacy_level privacylevel;
-	enum blocking_mode blockingmode;
-	enum refresh_hostnames refresh_hostnames;
 	bool socket_listenlocal;
 	bool analyze_AAAA;
 	bool resolveIPv6;
@@ -48,8 +44,19 @@ typedef struct {
 	bool block_esni;
 	bool names_from_netdb;
 	bool edns0_ecs;
+	enum privacy_level privacylevel;
+	enum blocking_mode blockingmode;
+	enum refresh_hostnames refresh_hostnames;
+	int maxDBdays;
+	int port;
+	int maxlogage;
+	int dns_port;
+	unsigned int delay_startup;
+	unsigned int network_expire;
+	enum debug_flags debug;
 	time_t DBinterval;
 } ConfigStruct;
+ASSERT_SIZEOF(ConfigStruct, 56, 48, 48);
 
 typedef struct {
 	const char* conf;

src/database/aliasclients.c

@@ -63,7 +63,7 @@ static void recompute_aliasclient(const int aliasclientID)
 		// Get pointer to client candidate
 		const clientsData *client = getClient(clientID, true);
 		// Skip invalid clients and alias-clients
-		if(client == NULL || client->aliasclient)
+		if(client == NULL || client->flags.aliasclient)
 			continue;
 
 		// Skip clients that are not managed by this aliasclient
@@ -131,7 +131,7 @@ bool import_aliasclients(void)
 		const int clientID = findClientID(aliasclient_str, false, true);
 
 		clientsData *client = getClient(clientID, true);
-		client->new = false;
+		client->flags.new = false;
 
 		// Reset counter
 		client->count = 0;
@@ -141,7 +141,7 @@ bool import_aliasclients(void)
 		client->namepos = addstr(name);
 
 		// This is a aliasclient
-		client->aliasclient = true;
+		client->flags.aliasclient = true;
 		client->aliasclient_id = aliasclient_id;
 
 		// Debug logging
@@ -169,7 +169,7 @@ bool import_aliasclients(void)
 static int get_aliasclient_ID(const clientsData *client)
 {
 	// Skip alias-clients themselves
-	if(client->aliasclient)
+	if(client->flags.aliasclient)
 		return -1;
 
 	const char *clientIP = getstr(client->ippos);
@@ -190,7 +190,7 @@ static int get_aliasclient_ID(const clientsData *client)
 		const clientsData *alias_client = getClient(aliasclientID, true);
 
 		// Skip clients that are not alias-clients
-		if(!alias_client->aliasclient)
+		if(!alias_client->flags.aliasclient)
 			continue;
 
 		// Compare MAC address of the current client to the
@@ -220,7 +220,7 @@ static int get_aliasclient_ID(const clientsData *client)
 void reset_aliasclient(clientsData *client)
 {
 	// Skip alias-clients themselves
-	if(client->aliasclient)
+	if(client->flags.aliasclient)
 		return;
 
 	// Find corresponding alias-client (if any)
@@ -288,7 +288,7 @@ void reimport_aliasclients(void)
 		// Get pointer to client candidate
 		clientsData *client = getClient(clientID, true);
 		// Skip invalid and non-alias-clients
-		if(client == NULL || !client->aliasclient)
+		if(client == NULL || !client->flags.aliasclient)
 			continue;
 
 		// Reset this alias-client
@@ -309,7 +309,7 @@ void reimport_aliasclients(void)
 		// Get pointer to client candidate
 		clientsData *client = getClient(clientID, true);
 		// Skip invalid and alias-clients
-		if(client == NULL || client->aliasclient)
+		if(client == NULL || client->flags.aliasclient)
 			continue;
 
 		reset_aliasclient(client);

src/database/gravity-db.c

@@ -30,7 +30,7 @@
 // reset_aliasclient()
 #include "aliasclients.h"
 
-// Definition of struct regex_data
+// Definition of struct regexData
 #include "../regex_r.h"
 
 // Prefix of interface names in the client table
@@ -219,7 +219,7 @@ static inline const char *show_client_string(const char *hwaddr, const char *hos
 static bool get_client_groupids(clientsData* client)
 {
 	const char *ip = getstr(client->ippos);
-	client->found_group = false;
+	client->flags.found_group = false;
 	client->groupspos = 0u;
 
 	// Do not proceed when database is not available
@@ -619,7 +619,7 @@ static bool get_client_groupids(clientsData* client)
 			     show_client_string(hwaddr, hostname, ip));
 
 		client->groupspos = addstr("0");
-		client->found_group = true;
+		client->flags.found_group = true;
 
 		if(hwaddr != NULL)
 		{
@@ -682,15 +682,15 @@ static bool get_client_groupids(clientsData* client)
 		if(result != NULL)
 		{
 			client->groupspos = addstr(result);
-			client->found_group = true;
+			client->flags.found_group = true;
 		}
 	}
 	else if(rc == SQLITE_DONE)
 	{
 		// Found no record for this client in the database
 		// -> No associated groups
 		client->groupspos = addstr("");
-		client->found_group = true;
+		client->flags.found_group = true;
 	}
 	else
 	{
@@ -805,7 +805,7 @@ bool gravityDB_prepare_client_statements(clientsData *client)
 
 	// Get associated groups for this client (if defined)
 	char *querystr = NULL;
-	if(!client->found_group && !get_client_groupids(client))
+	if(!client->flags.found_group && !get_client_groupids(client))
 		return false;
 
 	// Prepare whitelist statement
@@ -888,7 +888,7 @@ static inline void gravityDB_finalize_client_statements(clientsData *client)
 	// client sends a query
 	if(client != NULL)
 	{
-		client->found_group = false;
+		client->flags.found_group = false;
 	}
 }
 
@@ -1304,14 +1304,14 @@ bool in_auditlist(const char *domain)
 	return domain_in_list(domain, auditlist_stmt, "auditlist");
 }
 
-bool gravityDB_get_regex_client_groups(clientsData* client, const unsigned int numregex, const regex_data *regex,
+bool gravityDB_get_regex_client_groups(clientsData* client, const unsigned int numregex, const regexData *regex,
                                        const unsigned char type, const char* table)
 {
 	if(config.debug & DEBUG_REGEX)
 		logg("Getting regex client groups for client with ID %i", client->id);
 
 	char *querystr = NULL;
-	if(!client->found_group && !get_client_groupids(client))
+	if(!client->flags.found_group && !get_client_groupids(client))
 		return false;
 
 	// Group filtering

src/database/gravity-db.h

@@ -12,7 +12,7 @@
 
 // clientsData
 #include "../datastructure.h"
-// regex_data
+// regexData
 #include "../regex_r.h"
 
 // Table indices
@@ -35,7 +35,7 @@ bool in_gravity(const char *domain, clientsData* client);
 bool in_blacklist(const char *domain, clientsData* client);
 bool in_whitelist(const char *domain, const DNSCacheData *dns_cache, clientsData* client);
 
-bool gravityDB_get_regex_client_groups(clientsData* client, const unsigned int numregex, const regex_data *regex,
+bool gravityDB_get_regex_client_groups(clientsData* client, const unsigned int numregex, const regexData *regex,
                                        const unsigned char type, const char* table);
 
 #endif //GRAVITY_H

src/database/network-table.c

@@ -689,7 +689,7 @@ static bool add_FTL_clients_to_network_table(enum arp_status *client_status, tim
 		}
 
 		// Silently skip alias-clients - they do not really exist
-		if(client->aliasclient)
+		if(client->flags.aliasclient)
 			continue;
 
 		// Get hostname and IP address of this client