From 15942b17cebe00825d535ea8e77f5487889d8eae Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 18 Aug 2022 22:42:37 +0200 Subject: [PATCH 1/5] Store domainlist IDs for blocked/permitted queries Signed-off-by: DL6ER --- src/api/api.c | 9 +++--- src/database/gravity-db.c | 59 ++++++++++++++------------------------ src/database/gravity-db.h | 2 +- src/database/query-table.c | 52 ++++++++++++++++----------------- src/datastructure.c | 1 + src/datastructure.h | 2 +- src/dnsmasq_interface.c | 15 +++++----- src/regex.c | 22 ++++++++++++-- src/regex_r.h | 3 +- test/run.sh | 3 ++ test/test_suite.bats | 32 ++++++++++----------- 11 files changed, 101 insertions(+), 99 deletions(-) diff --git a/src/api/api.c b/src/api/api.c index 1c2a7265..67da3334 100644 --- a/src/api/api.c +++ b/src/api/api.c @@ -1009,14 +1009,13 @@ void getAllQueries(const char *client_message, const int sock, const bool isteln } // Get ID of blocking regex, if applicable and permitted by privacy settings - int regex_idx = -1; - if ((query->status == QUERY_REGEX || query->status == QUERY_REGEX_CNAME) && - config.privacylevel < PRIVACY_HIDE_DOMAINS) + int domainlist_id = -1; + if (config.privacylevel < PRIVACY_HIDE_DOMAINS) { unsigned int cacheID = findCacheID(query->domainID, query->clientID, query->type); DNSCacheData *dns_cache = getDNSCache(cacheID, true); if(dns_cache != NULL) - regex_idx = dns_cache->black_regex_idx; + domainlist_id = dns_cache->domainlist_id; } // Get IP of upstream destination, if applicable @@ -1065,7 +1064,7 @@ void getAllQueries(const char *client_message, const int sock, const bool isteln reply, delay, CNAME_domain, - regex_idx, + domainlist_id, upstream_name, upstream_port, query->ede == -1 ? "" : get_edestr(query->ede)); diff --git a/src/database/gravity-db.c b/src/database/gravity-db.c index 06551dfe..d4c5d6d0 100644 --- a/src/database/gravity-db.c +++ b/src/database/gravity-db.c @@ -15,8 +15,6 @@ #include "../config.h" // logg() #include "../log.h" -// match_regex() -#include "../regex_r.h" // getstr() #include "../shmem.h" // SQLite3 prepared statement vectors @@ -210,11 +208,11 @@ bool gravityDB_reopen(void) return gravityDB_open(); } -static char* get_client_querystr(const char* table, const char* groups) +static char* get_client_querystr(const char *table, const char *column, const char *groups) { // Build query string with group filtering char *querystr = NULL; - if(asprintf(&querystr, "SELECT EXISTS(SELECT domain from %s WHERE domain = ? AND group_id IN (%s));", table, groups) < 1) + if(asprintf(&querystr, "SELECT %s from %s WHERE domain = ? AND group_id IN (%s);", column, table, groups) < 1) { logg("get_client_querystr(%s, %s) - asprintf() error", table, groups); return NULL; @@ -858,19 +856,14 @@ bool gravityDB_prepare_client_statements(clientsData *client) return false; // Prepare whitelist statement - // We use SELECT EXISTS() as this is known to efficiently use the index - // We are only interested in whether the domain exists or not in the - // list but don't case about duplicates or similar. SELECT EXISTS(...) - // returns true as soon as it sees the first row from the query inside - // of EXISTS(). if(config.debug & DEBUG_DATABASE) logg("gravityDB_open(): Preparing vw_whitelist statement for client %s", clientip); - querystr = get_client_querystr("vw_whitelist", getstr(client->groupspos)); + querystr = get_client_querystr("vw_whitelist", "id", getstr(client->groupspos)); sqlite3_stmt* stmt = NULL; int rc = sqlite3_prepare_v3(gravity_db, querystr, -1, SQLITE_PREPARE_PERSISTENT, &stmt, NULL); if( rc != SQLITE_OK ) { - logg("gravityDB_open(\"SELECT EXISTS(... vw_whitelist ...)\") - SQL error prepare: %s", sqlite3_errstr(rc)); + logg("gravityDB_open(\"SELECT(... vw_whitelist ...)\") - SQL error prepare: %s", sqlite3_errstr(rc)); gravityDB_close(); return false; } @@ -880,11 +873,11 @@ bool gravityDB_prepare_client_statements(clientsData *client) // Prepare gravity statement if(config.debug & DEBUG_DATABASE) logg("gravityDB_open(): Preparing vw_gravity statement for client %s", clientip); - querystr = get_client_querystr("vw_gravity", getstr(client->groupspos)); + querystr = get_client_querystr("vw_gravity", "domain", getstr(client->groupspos)); rc = sqlite3_prepare_v3(gravity_db, querystr, -1, SQLITE_PREPARE_PERSISTENT, &stmt, NULL); if( rc != SQLITE_OK ) { - logg("gravityDB_open(\"SELECT EXISTS(... vw_gravity ...)\") - SQL error prepare: %s", sqlite3_errstr(rc)); + logg("gravityDB_open(\"SELECT(... vw_gravity ...)\") - SQL error prepare: %s", sqlite3_errstr(rc)); gravityDB_close(); return false; } @@ -894,11 +887,11 @@ bool gravityDB_prepare_client_statements(clientsData *client) // Prepare blacklist statement if(config.debug & DEBUG_DATABASE) logg("gravityDB_open(): Preparing vw_blacklist statement for client %s", clientip); - querystr = get_client_querystr("vw_blacklist", getstr(client->groupspos)); + querystr = get_client_querystr("vw_blacklist", "id", getstr(client->groupspos)); rc = sqlite3_prepare_v3(gravity_db, querystr, -1, SQLITE_PREPARE_PERSISTENT, &stmt, NULL); if( rc != SQLITE_OK ) { - logg("gravityDB_open(\"SELECT EXISTS(... vw_blacklist ...)\") - SQL error prepare: %s", sqlite3_errstr(rc)); + logg("gravityDB_open(\"SELECT(... vw_blacklist ...)\") - SQL error prepare: %s", sqlite3_errstr(rc)); gravityDB_close(); return false; } @@ -1146,7 +1139,7 @@ int gravityDB_count(const enum gravity_tables list) return result; } -static enum db_result domain_in_list(const char *domain, sqlite3_stmt *stmt, const char *listname) +static enum db_result domain_in_list(const char *domain, sqlite3_stmt *stmt, const char *listname, int *domain_id) { // Do not try to bind text to statement when database is not available if(!gravityDB_opened && !gravityDB_open()) @@ -1180,10 +1173,10 @@ static enum db_result domain_in_list(const char *domain, sqlite3_stmt *stmt, con sqlite3_clear_bindings(stmt); return LIST_NOT_AVAILABLE; } - else if(rc != SQLITE_ROW) + else if(rc != SQLITE_ROW && rc != SQLITE_DONE) { - // Any return code that is neither SQLITE_BUSY not SQLITE_ROW - // is a real error we should log + // Any return code that is neither SQLITE_BUSY nor SQLITE_ROW or + // SQLITE_DONE is an error we should log logg("domain_in_list(\"%s\", %p, %s): Failed to perform step: %s", domain, stmt, listname, sqlite3_errstr(rc)); sqlite3_reset(stmt); @@ -1191,8 +1184,10 @@ static enum db_result domain_in_list(const char *domain, sqlite3_stmt *stmt, con return LIST_NOT_AVAILABLE; } - // Get result of query "SELECT EXISTS(...)" - const int result = sqlite3_column_int(stmt, 0); + // Get result of query (if available) + const int result = (rc == SQLITE_ROW) ? sqlite3_column_int(stmt, 0) : -1; + if(domain_id != NULL) + *domain_id = result; if(config.debug & DEBUG_DATABASE) logg("domain_in_list(\"%s\", %p, %s): %d", domain, stmt, listname, result); @@ -1210,7 +1205,7 @@ static enum db_result domain_in_list(const char *domain, sqlite3_stmt *stmt, con // Return if domain was found in current table // SELECT EXISTS(...) either returns 0 (false) or 1 (true). - return (result == 1) ? FOUND : NOT_FOUND; + return (rc == SQLITE_ROW) ? FOUND : NOT_FOUND; } void gravityDB_reload_groups(clientsData* client) @@ -1268,17 +1263,7 @@ enum db_result in_whitelist(const char *domain, DNSCacheData *dns_cache, clients // We have to check both the exact whitelist (using a prepared database statement) // as well the compiled regex whitelist filters to check if the current domain is // whitelisted. - enum db_result on_whitelist = domain_in_list(domain, stmt, "whitelist"); - - // For performance reasons, the regex evaluations is executed only if the - // exact whitelist lookup does not deliver a positive match. This is an - // optimization as the database lookup will most likely hit (a) more domains - // and (b) will be faster (given a sufficiently large number of regex - // whitelisting filters). - if(on_whitelist == NOT_FOUND) - on_whitelist = match_regex(domain, dns_cache, client->id, REGEX_WHITELIST, false) != -1; - - return on_whitelist; + return domain_in_list(domain, stmt, "whitelist", &dns_cache->domainlist_id); } enum db_result in_gravity(const char *domain, clientsData *client) @@ -1306,10 +1291,10 @@ enum db_result in_gravity(const char *domain, clientsData *client) if(stmt == NULL) stmt = gravity_stmt->get(gravity_stmt, client->id); - return domain_in_list(domain, stmt, "gravity"); + return domain_in_list(domain, stmt, "gravity", NULL); } -enum db_result in_blacklist(const char *domain, clientsData *client) +enum db_result in_blacklist(const char *domain, DNSCacheData *dns_cache, clientsData *client) { // If list statement is not ready and cannot be initialized (e.g. no // access to the database), we return false to prevent an FTL crash @@ -1334,7 +1319,7 @@ enum db_result in_blacklist(const char *domain, clientsData *client) if(stmt == NULL) stmt = blacklist_stmt->get(blacklist_stmt, client->id); - return domain_in_list(domain, stmt, "blacklist"); + return domain_in_list(domain, stmt, "blacklist", &dns_cache->domainlist_id); } bool in_auditlist(const char *domain) @@ -1345,7 +1330,7 @@ bool in_auditlist(const char *domain) return false; // We check the domain_audit table for the given domain - return domain_in_list(domain, auditlist_stmt, "auditlist") == FOUND; + return domain_in_list(domain, auditlist_stmt, "auditlist", NULL) == FOUND; } bool gravityDB_get_regex_client_groups(clientsData* client, const unsigned int numregex, const regexData *regex, diff --git a/src/database/gravity-db.h b/src/database/gravity-db.h index 6c4b9f00..84beb1c8 100644 --- a/src/database/gravity-db.h +++ b/src/database/gravity-db.h @@ -31,7 +31,7 @@ void gravityDB_finalizeTable(void); int gravityDB_count(const enum gravity_tables list); enum db_result in_gravity(const char *domain, clientsData *client); -enum db_result in_blacklist(const char *domain, clientsData *client); +enum db_result in_blacklist(const char *domain, DNSCacheData *dns_cache, clientsData *client); enum db_result in_whitelist(const char *domain, DNSCacheData *dns_cache, clientsData *client); bool in_auditlist(const char *domain); diff --git a/src/database/query-table.c b/src/database/query-table.c index 6c7986ab..3b50b66f 100644 --- a/src/database/query-table.c +++ b/src/database/query-table.c @@ -373,6 +373,9 @@ int DB_save_queries(sqlite3 *db) sqlite3_bind_null(query_stmt, 7); } + const int cacheID = findCacheID(query->domainID, query->clientID, query->type); + DNSCacheData *cache = getDNSCache(cacheID, true); + // ADDITIONAL_INFO if(query->status == QUERY_GRAVITY_CNAME || query->status == QUERY_REGEX_CNAME || @@ -389,37 +392,29 @@ int DB_save_queries(sqlite3 *db) sqlite3_bind_text(addinfo_stmt, 2, cname, len, SQLITE_STATIC); if(sqlite3_step(addinfo_stmt) != SQLITE_DONE) { - logg("Encountered error while trying to store addinfo in long-term database"); + logg("Encountered error while trying to store addinfo in long-term database (CNAME)"); error = true; break; } sqlite3_clear_bindings(addinfo_stmt); sqlite3_reset(addinfo_stmt); } - else if(query->status == QUERY_REGEX) + else if(cache != NULL && cache->domainlist_id > -1) { - // Restore regex ID if applicable - const int cacheID = findCacheID(query->domainID, query->clientID, query->type); - DNSCacheData *cache = getDNSCache(cacheID, true); - if(cache != NULL) - { - sqlite3_bind_int(query_stmt, 8, ADDINFO_REGEX_ID); - sqlite3_bind_int(query_stmt, 9, cache->black_regex_idx); + sqlite3_bind_int(query_stmt, 8, ADDINFO_REGEX_ID); + sqlite3_bind_int(query_stmt, 9, cache->domainlist_id); - // Execute prepared addinfo statement and check if successful - sqlite3_bind_int(addinfo_stmt, 1, ADDINFO_REGEX_ID); - sqlite3_bind_int(addinfo_stmt, 2, cache->black_regex_idx); - if(sqlite3_step(addinfo_stmt) != SQLITE_DONE) - { - logg("Encountered error while trying to store addinfo in long-term database"); - error = true; - break; - } - sqlite3_clear_bindings(addinfo_stmt); - sqlite3_reset(addinfo_stmt); + // Execute prepared addinfo statement and check if successful + sqlite3_bind_int(addinfo_stmt, 1, ADDINFO_REGEX_ID); + sqlite3_bind_int(addinfo_stmt, 2, cache->domainlist_id); + if(sqlite3_step(addinfo_stmt) != SQLITE_DONE) + { + logg("Encountered error while trying to store addinfo in long-term database (domainlist_id)"); + error = true; + break; } - else - sqlite3_bind_null(query_stmt, 8); + sqlite3_clear_bindings(addinfo_stmt); + sqlite3_reset(addinfo_stmt); } else { @@ -933,16 +928,17 @@ void DB_read_queries(void) query->CNAME_domainID = CNAMEdomainID; } } - else if(status == QUERY_REGEX) + else if(sqlite3_column_bytes(stmt, 7) != 0) { - // QUERY_REGEX: Set ID regex which was the reason for blocking + // Set ID of the domainlist entry that was the reason for permitting/blocking this query + // We assume the value in this field is said ID when it is not a CNAME-related domain + // (checked above) and the value of additional_info is not NULL (0 bytes storage size) const int cacheID = findCacheID(query->domainID, query->clientID, query->type); DNSCacheData *cache = getDNSCache(cacheID, true); // Only load if - // a) we have a chace entry - // b) the value of additional_info is not NULL (0 bytes storage size) - if(cache != NULL && sqlite3_column_bytes(stmt, 7) != 0) - cache->black_regex_idx = sqlite3_column_int(stmt, 7); + // a) we have a cache entry + if(cache != NULL) + cache->domainlist_id = sqlite3_column_int(stmt, 7); } // Increment status counters, we first have to add one to the count of diff --git a/src/datastructure.c b/src/datastructure.c index d7bc1cfd..af6b1cc1 100644 --- a/src/datastructure.c +++ b/src/datastructure.c @@ -358,6 +358,7 @@ int findCacheID(int domainID, int clientID, enum query_types query_type) dns_cache->clientID = clientID; dns_cache->query_type = query_type; dns_cache->force_reply = 0u; + dns_cache->domainlist_id = -1; // -1 = not set // Increase counter by one counters->dns_cache_size++; diff --git a/src/datastructure.h b/src/datastructure.h index ee997001..98876cc1 100644 --- a/src/datastructure.h +++ b/src/datastructure.h @@ -104,7 +104,7 @@ typedef struct { enum query_types query_type; int domainID; int clientID; - int black_regex_idx; + int domainlist_id; } DNSCacheData; void strtolower(char *str); diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c index dddd4e89..dc2285b7 100644 --- a/src/dnsmasq_interface.c +++ b/src/dnsmasq_interface.c @@ -1089,7 +1089,7 @@ static bool check_domain_blocked(const char *domain, const int clientID, return false; // Check domains against exact blacklist - enum db_result blacklist = in_blacklist(domain, client); + enum db_result blacklist = in_blacklist(domain, dns_cache, client); if(blacklist == FOUND) { // Set new status @@ -1158,8 +1158,7 @@ static bool check_domain_blocked(const char *domain, const int clientID, // Check domain against blacklist regex filters // Skipped when the domain is whitelisted or blocked by exact blacklist or gravity - int regex_idx = 0; - if((regex_idx = match_regex(domain, dns_cache, client->id, REGEX_BLACKLIST, false)) > -1) + if(in_regex(domain, dns_cache, client-> id, REGEX_BLACKLIST) == FOUND) { // Set new status *new_status = QUERY_REGEX; @@ -1167,14 +1166,13 @@ static bool check_domain_blocked(const char *domain, const int clientID, // Mark domain as regex matched for this client set_dnscache_blockingstatus(dns_cache, client, REGEX_BLOCKED, domain); - dns_cache->black_regex_idx = regex_idx; // Regex may be overwriting reply type for this domain if(dns_cache->force_reply != REPLY_UNKNOWN) force_next_DNS_reply = dns_cache->force_reply; // Store ID of this regex (fork-private) - last_regex_idx = regex_idx; + last_regex_idx = dns_cache->domainlist_id; // We block this domain return true; @@ -1330,7 +1328,7 @@ static bool _FTL_check_blocking(int queryID, int domainID, int clientID, const c if(!query->flags.whitelisted) { force_next_DNS_reply = dns_cache->force_reply; - last_regex_idx = dns_cache->black_regex_idx; + last_regex_idx = dns_cache->domainlist_id; query_blocked(query, domain, client, QUERY_REGEX); return true; } @@ -1414,6 +1412,9 @@ static bool _FTL_check_blocking(int queryID, int domainID, int clientID, const c // Check whitelist (exact + regex) for match query->flags.whitelisted = in_whitelist(domainstr, dns_cache, client) == FOUND; + if(!query->flags.whitelisted) + query->flags.whitelisted = in_regex(domainstr, dns_cache, client->id, REGEX_WHITELIST) == FOUND; + // Check blacklist (exact + regex) and gravity for queried domain unsigned char new_status = QUERY_UNKNOWN; bool db_okay = true; @@ -1589,7 +1590,7 @@ bool _FTL_CNAME(const char *domain, const struct crec *cpp, const int id, const // Propagate ID of responsible regex up from the child to the parent domain if(parent_cache != NULL && child_cache != NULL) { - child_cache->black_regex_idx = parent_cache->black_regex_idx; + child_cache->domainlist_id = parent_cache->domainlist_id; } // Set status diff --git a/src/regex.c b/src/regex.c index 23f5c2bf..2a46dd70 100644 --- a/src/regex.c +++ b/src/regex.c @@ -302,8 +302,8 @@ static bool compile_regex(const char *regexin, const enum regex_type regexid, co return true; } -int match_regex(const char *input, DNSCacheData* dns_cache, const int clientID, - const enum regex_type regexid, const bool regextest) +static int match_regex(const char *input, DNSCacheData* dns_cache, const int clientID, + const enum regex_type regexid, const bool regextest) { int match_idx = -1; regexData *regex = get_regex_ptr(regexid); @@ -475,6 +475,24 @@ int match_regex(const char *input, DNSCacheData* dns_cache, const int clientID, return match_idx; } +enum db_result in_regex(const char *domain, DNSCacheData *dns_cache, const int clientID, const enum regex_type regexid) +{ + // For performance reasons, the regex evaluations is executed only if the + // exact whitelist lookup does not deliver a positive match. This is an + // optimization as the database lookup will most likely hit (a) more domains + // and (b) will be faster (given a sufficiently large number of regex + // whitelisting filters). + const int regex_id = match_regex(domain, dns_cache, clientID, regexid, false); + if(regex_id != -1) + { + // We found a match + dns_cache->domainlist_id = regex_id; + return FOUND; + } + + return NOT_FOUND; +} + static void free_regex(void) { // Return early if we don't use any regex filters diff --git a/src/regex_r.h b/src/regex_r.h index 6a44b4d5..6a63dcda 100644 --- a/src/regex_r.h +++ b/src/regex_r.h @@ -44,8 +44,7 @@ typedef struct { } regexData; unsigned int get_num_regex(const enum regex_type regexid) __attribute__((pure)); -int match_regex(const char *input, DNSCacheData* dns_cache, const int clientID, - const enum regex_type regexid, const bool regextest); +enum db_result in_regex(const char *domain, DNSCacheData *dns_cache, const int clientID, const enum regex_type regexid); void allocate_regex_client_enabled(clientsData *client, const int clientID); void reload_per_client_regex(clientsData *client); void read_regex_from_database(void); diff --git a/test/run.sh b/test/run.sh index 63d75d7f..17901625 100755 --- a/test/run.sh +++ b/test/run.sh @@ -108,6 +108,9 @@ if [[ $RET != 0 ]]; then echo "" echo -n "ptr.log: " curl_to_tricorder ./ptr.log + echo ""getallqueries + echo -n "getallqueries.log: " + curl_to_tricorder ./getallqueries.log echo "" fi diff --git a/test/test_suite.bats b/test/test_suite.bats index 1931e6b4..091c1466 100644 --- a/test/test_suite.bats +++ b/test/test_suite.bats @@ -171,13 +171,13 @@ run bash -c "grep -c 'Regex whitelist: Querying groups for client 127.0.0.4: \"SELECT id from vw_regex_whitelist WHERE group_id IN (4);\"' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} == "1" ]] - run bash -c "grep -c 'get_client_querystr: SELECT EXISTS(SELECT domain from vw_whitelist WHERE domain = ? AND group_id IN (4));' /var/log/pihole/FTL.log" + run bash -c "grep -c 'get_client_querystr: SELECT id from vw_whitelist WHERE domain = ? AND group_id IN (4);' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} != "0" ]] - run bash -c "grep -c 'get_client_querystr: SELECT EXISTS(SELECT domain from vw_blacklist WHERE domain = ? AND group_id IN (4));' /var/log/pihole/FTL.log" + run bash -c "grep -c 'get_client_querystr: SELECT id from vw_blacklist WHERE domain = ? AND group_id IN (4);' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} != "0" ]] - run bash -c "grep -c 'get_client_querystr: SELECT EXISTS(SELECT domain from vw_gravity WHERE domain = ? AND group_id IN (4));' /var/log/pihole/FTL.log" + run bash -c "grep -c 'get_client_querystr: SELECT domain from vw_gravity WHERE domain = ? AND group_id IN (4);' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} != "0" ]] run bash -c "grep -c 'Regex whitelist ([[:digit:]]*, DB ID [[:digit:]]*) .* NOT ENABLED for client 127.0.0.4' /var/log/pihole/FTL.log" @@ -203,13 +203,13 @@ run bash -c "grep -c 'Regex whitelist: Querying groups for client 127.0.0.5: \"SELECT id from vw_regex_whitelist WHERE group_id IN (4);\"' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} == "1" ]] - run bash -c "grep -c 'get_client_querystr: SELECT EXISTS(SELECT domain from vw_whitelist WHERE domain = ? AND group_id IN (4));' /var/log/pihole/FTL.log" + run bash -c "grep -c 'get_client_querystr: SELECT id from vw_whitelist WHERE domain = ? AND group_id IN (4);' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} != "0" ]] - run bash -c "grep -c 'get_client_querystr: SELECT EXISTS(SELECT domain from vw_blacklist WHERE domain = ? AND group_id IN (4));' /var/log/pihole/FTL.log" + run bash -c "grep -c 'get_client_querystr: SELECT id from vw_blacklist WHERE domain = ? AND group_id IN (4);' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} != "0" ]] - run bash -c "grep -c 'get_client_querystr: SELECT EXISTS(SELECT domain from vw_gravity WHERE domain = ? AND group_id IN (4));' /var/log/pihole/FTL.log" + run bash -c "grep -c 'get_client_querystr: SELECT domain from vw_gravity WHERE domain = ? AND group_id IN (4);' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} != "0" ]] run bash -c "grep -c 'Regex whitelist ([[:digit:]]*, DB ID [[:digit:]]*) .* NOT ENABLED for client 127.0.0.5' /var/log/pihole/FTL.log" @@ -241,13 +241,13 @@ run bash -c "grep -c 'Regex whitelist: Querying groups for client 127.0.0.6: \"SELECT id from vw_regex_whitelist WHERE group_id IN (5);\"' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} == "1" ]] - run bash -c "grep -c 'get_client_querystr: SELECT EXISTS(SELECT domain from vw_whitelist WHERE domain = ? AND group_id IN (5));' /var/log/pihole/FTL.log" + run bash -c "grep -c 'get_client_querystr: SELECT id from vw_whitelist WHERE domain = ? AND group_id IN (5);' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} == "1" ]] - run bash -c "grep -c 'get_client_querystr: SELECT EXISTS(SELECT domain from vw_blacklist WHERE domain = ? AND group_id IN (5));' /var/log/pihole/FTL.log" + run bash -c "grep -c 'get_client_querystr: SELECT id from vw_blacklist WHERE domain = ? AND group_id IN (5);' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} == "1" ]] - run bash -c "grep -c 'get_client_querystr: SELECT EXISTS(SELECT domain from vw_gravity WHERE domain = ? AND group_id IN (5));' /var/log/pihole/FTL.log" + run bash -c "grep -c 'get_client_querystr: SELECT domain from vw_gravity WHERE domain = ? AND group_id IN (5);' /var/log/pihole/FTL.log" printf "%s\n" "${lines[@]}" [[ ${lines[0]} == "1" ]] run bash -c "grep -c 'Regex whitelist ([[:digit:]]*, DB ID [[:digit:]]*) .* NOT ENABLED for client 127.0.0.6' /var/log/pihole/FTL.log" @@ -537,22 +537,22 @@ # Here and below: Reply time is varying. don't test for a particular value (..."*"...) @test "Get all queries shows expected content" { - run bash -c 'echo ">getallqueries >quit" | nc -v 127.0.0.1 4711' + run bash -c 'echo ">getallqueries >quit" | nc -v 127.0.0.1 4711 | tee getallqueries.log' printf "%s\n" "${lines[@]}" [[ ${lines[1]} == *" TXT version.ftl 127.0.0.1 3 2 6 "*" N/A -1 N/A#0 \"\" \"0\""* ]] [[ ${lines[2]} == *" TXT version.bind 127.0.0.1 3 2 6 "*" N/A -1 N/A#0 \"\" \"1\""* ]] - [[ ${lines[3]} == *" A blacklisted.ftl 127.0.0.1 5 2 4 "*" N/A -1 N/A#0 \"\" \"2\""* ]] + [[ ${lines[3]} == *" A blacklisted.ftl 127.0.0.1 5 2 4 "*" N/A 5 N/A#0 \"\" \"2\""* ]] [[ ${lines[4]} == *" A gravity.ftl 127.0.0.1 1 2 4 "*" N/A -1 N/A#0 \"\" \"3\""* ]] [[ ${lines[5]} == *" A gravity.ftl 127.0.0.1 1 2 4 "*" N/A -1 N/A#0 \"\" \"4\""* ]] - [[ ${lines[6]} == *" A whitelisted.ftl 127.0.0.1 2 2 4 "*" N/A -1 127.0.0.1#5555 \"\" \"5\""* ]] - [[ ${lines[7]} == *" A gravity-whitelisted.ftl 127.0.0.1 2 2 4 "*" N/A -1 127.0.0.1#5555 \"\" \"6\""* ]] + [[ ${lines[6]} == *" A whitelisted.ftl 127.0.0.1 2 2 4 "*" N/A 1 127.0.0.1#5555 \"\" \"5\""* ]] + [[ ${lines[7]} == *" A gravity-whitelisted.ftl 127.0.0.1 2 2 4 "*" N/A 4 127.0.0.1#5555 \"\" \"6\""* ]] [[ ${lines[8]} == *" A regex5.ftl 127.0.0.1 4 2 4 "*" N/A 6 N/A#0 \"\" \"7\""* ]] [[ ${lines[9]} == *" A regexa.ftl 127.0.0.1 2 2 4 "*" N/A -1 127.0.0.1#5555 \"\" \"8\""* ]] - [[ ${lines[10]} == *" A regex1.ftl 127.0.0.1 2 2 4 "*" N/A -1 127.0.0.1#5555 \"\" \"9\""* ]] - [[ ${lines[11]} == *" A regex2.ftl 127.0.0.1 2 2 4 "*" N/A -1 127.0.0.1#5555 \"\" \"10\""* ]] + [[ ${lines[10]} == *" A regex1.ftl 127.0.0.1 2 2 4 "*" N/A 2 127.0.0.1#5555 \"\" \"9\""* ]] + [[ ${lines[11]} == *" A regex2.ftl 127.0.0.1 2 2 4 "*" N/A 3 127.0.0.1#5555 \"\" \"10\""* ]] [[ ${lines[12]} == *" A whitelisted.ftl 127.0.0.2 1 2 4 "*" N/A -1 N/A#0 \"\" \"11\""* ]] [[ ${lines[13]} == *" A regex1.ftl 127.0.0.2 4 2 4 "*" N/A 6 N/A#0 \"\" \"12\""* ]] - [[ ${lines[14]} == *" A regex1.ftl 127.0.0.1 3 2 4 "*" N/A -1 N/A#0 \"\" \"13\""* ]] + [[ ${lines[14]} == *" A regex1.ftl 127.0.0.1 3 2 4 "*" N/A 2 N/A#0 \"\" \"13\""* ]] [[ ${lines[15]} == *" A regex1.ftl 127.0.0.3 3 2 4 "*" N/A -1 N/A#0 \"\" \"14\""* ]] [[ ${lines[16]} == *" A blacklisted.ftl 127.0.0.2 2 2 4 "*" N/A -1 127.0.0.1#5555 \"\" \"15\""* ]] [[ ${lines[17]} == *" A blacklisted.ftl 127.0.0.3 3 2 4 "*" N/A -1 N/A#0 \"\" \"16\""* ]] From da335175538227e5590791c33de4e30879e8956f Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 20 Aug 2022 14:23:21 +0200 Subject: [PATCH 2/5] Add more debugging output to getstr() Signed-off-by: DL6ER --- src/dnsmasq_interface.c | 7 ++++--- src/regex.c | 6 +++--- src/regex_r.h | 2 +- src/shmem.c | 6 +++--- src/shmem.h | 3 ++- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c index dc2285b7..4c31c9f2 100644 --- a/src/dnsmasq_interface.c +++ b/src/dnsmasq_interface.c @@ -1158,7 +1158,7 @@ static bool check_domain_blocked(const char *domain, const int clientID, // Check domain against blacklist regex filters // Skipped when the domain is whitelisted or blocked by exact blacklist or gravity - if(in_regex(domain, dns_cache, client-> id, REGEX_BLACKLIST) == FOUND) + if(in_regex(domain, dns_cache, client-> id, REGEX_BLACKLIST)) { // Set new status *new_status = QUERY_REGEX; @@ -1409,11 +1409,12 @@ static bool _FTL_check_blocking(int queryID, int domainID, int clientID, const c domainstr = strdup(domainstr); const char *blockedDomain = domainstr; - // Check whitelist (exact + regex) for match + // Check exact whitelist for match query->flags.whitelisted = in_whitelist(domainstr, dns_cache, client) == FOUND; + // If not found: Check regex whitelist for match if(!query->flags.whitelisted) - query->flags.whitelisted = in_regex(domainstr, dns_cache, client->id, REGEX_WHITELIST) == FOUND; + query->flags.whitelisted = in_regex(domainstr, dns_cache, client->id, REGEX_WHITELIST); // Check blacklist (exact + regex) and gravity for queried domain unsigned char new_status = QUERY_UNKNOWN; diff --git a/src/regex.c b/src/regex.c index 2a46dd70..e6753023 100644 --- a/src/regex.c +++ b/src/regex.c @@ -475,7 +475,7 @@ static int match_regex(const char *input, DNSCacheData* dns_cache, const int cli return match_idx; } -enum db_result in_regex(const char *domain, DNSCacheData *dns_cache, const int clientID, const enum regex_type regexid) +bool in_regex(const char *domain, DNSCacheData *dns_cache, const int clientID, const enum regex_type regexid) { // For performance reasons, the regex evaluations is executed only if the // exact whitelist lookup does not deliver a positive match. This is an @@ -487,10 +487,10 @@ enum db_result in_regex(const char *domain, DNSCacheData *dns_cache, const int c { // We found a match dns_cache->domainlist_id = regex_id; - return FOUND; + return true; } - return NOT_FOUND; + return false; } static void free_regex(void) diff --git a/src/regex_r.h b/src/regex_r.h index 6a63dcda..7ec979f7 100644 --- a/src/regex_r.h +++ b/src/regex_r.h @@ -44,7 +44,7 @@ typedef struct { } regexData; unsigned int get_num_regex(const enum regex_type regexid) __attribute__((pure)); -enum db_result in_regex(const char *domain, DNSCacheData *dns_cache, const int clientID, const enum regex_type regexid); +bool in_regex(const char *domain, DNSCacheData *dns_cache, const int clientID, const enum regex_type regexid); void allocate_regex_client_enabled(clientsData *client, const int clientID); void reload_per_client_regex(clientsData *client); void read_regex_from_database(void); diff --git a/src/shmem.c b/src/shmem.c index b0fcbc6a..648b3f44 100644 --- a/src/shmem.c +++ b/src/shmem.c @@ -272,14 +272,14 @@ size_t addstr(const char *input) return (shmSettings->next_str_pos - len); } -const char *getstr(const size_t pos) +const char *_getstr(const size_t pos, const char *func, const int line, const char *file) { // Only access the string memory if this memory region has already been set if(pos < shmSettings->next_str_pos) return &((const char*)shm_strings.ptr)[pos]; else { - logg("WARN: Tried to access %zu but next_str_pos is %u", pos, shmSettings->next_str_pos); + logg("WARN: Tried to access %zu in %s() (%s:%i) but next_str_pos is %u", pos, func, file, line, shmSettings->next_str_pos); return ""; } } @@ -337,7 +337,7 @@ static void remap_shm(void) } // Obtain SHMEM lock -void _lock_shm(const char* func, const int line, const char * file) +void _lock_shm(const char *func, const int line, const char *file) { if(config.debug & DEBUG_LOCKS) logg("Waiting for SHM lock in %s() (%s:%i)", func, file, line); diff --git a/src/shmem.h b/src/shmem.h index 9c59bed3..4532c6d2 100644 --- a/src/shmem.h +++ b/src/shmem.h @@ -102,7 +102,8 @@ void _unlock_log(const char* func, const int line, const char * file); bool init_shmem(bool create_new); void destroy_shmem(void); size_t addstr(const char *str); -const char *getstr(const size_t pos); +#define getstr(pos) _getstr(pos, __FUNCTION__, __LINE__, __FILE__) +const char *_getstr(const size_t pos, const char *func, const int line, const char *file); /** * Escapes a string by replacing special characters, such as spaces From ff30d4a97361ffd301bd322edc9e0a4ddf2c5318 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 20 Aug 2022 14:30:48 +0200 Subject: [PATCH 3/5] Log memory access error details even when no debug flag is set Signed-off-by: DL6ER --- src/datastructure.c | 16 ++++++------- src/datastructure.h | 17 ++++++------- src/shmem.c | 58 ++++++++++++++++++++------------------------- 3 files changed, 43 insertions(+), 48 deletions(-) diff --git a/src/datastructure.c b/src/datastructure.c index af6b1cc1..e1aa775a 100644 --- a/src/datastructure.c +++ b/src/datastructure.c @@ -319,13 +319,13 @@ void change_clientcount(clientsData *client, int total, int blocked, int overTim } } -int findCacheID(int domainID, int clientID, enum query_types query_type) +int _findCacheID(int domainID, int clientID, enum query_types query_type, const char *func, int line, const char *file) { // Compare content of client against known client IP addresses for(int cacheID = 0; cacheID < counters->dns_cache_size; cacheID++) { // Get cache pointer - DNSCacheData* dns_cache = getDNSCache(cacheID, true); + DNSCacheData* dns_cache = _getDNSCache(cacheID, true, line, func, file); // Check if the returned pointer is valid before trying to access it if(dns_cache == NULL) @@ -343,7 +343,7 @@ int findCacheID(int domainID, int clientID, enum query_types query_type) const int cacheID = counters->dns_cache_size; // Get client pointer - DNSCacheData* dns_cache = getDNSCache(cacheID, false); + DNSCacheData* dns_cache = _getDNSCache(cacheID, false, line, func, file); if(dns_cache == NULL) { @@ -563,7 +563,7 @@ static const char *query_status_str[QUERY_STATUS_MAX] = { "SPECIAL_DOMAIN" }; -void _query_set_status(queriesData *query, const enum query_status new_status, const char *file, const int line) +void _query_set_status(queriesData *query, const enum query_status new_status, const char *func, const int line, const char *file) { // Debug logging if(config.debug & DEBUG_STATUS) @@ -571,14 +571,14 @@ void _query_set_status(queriesData *query, const enum query_status new_status, c const char *oldstr = query->status < QUERY_STATUS_MAX ? query_status_str[query->status] : "INVALID"; if(query->status == new_status) { - logg("Query %i: status unchanged: %s (%d) in %s:%i", - query->id, oldstr, query->status, short_path(file), line); + logg("Query %i: status unchanged: %s (%d) in %s() (%s:%i)", + query->id, oldstr, query->status, func, short_path(file), line); } else { const char *newstr = new_status < QUERY_STATUS_MAX ? query_status_str[new_status] : "INVALID"; - logg("Query %i: status changed: %s (%d) -> %s (%d) in %s:%i", - query->id, oldstr, query->status, newstr, new_status, short_path(file), line); + logg("Query %i: status changed: %s (%d) -> %s (%d) in %s() (%s:%i)", + query->id, oldstr, query->status, newstr, new_status, func, short_path(file), line); } } diff --git a/src/datastructure.h b/src/datastructure.h index 98876cc1..e1ef2779 100644 --- a/src/datastructure.h +++ b/src/datastructure.h @@ -113,13 +113,14 @@ int findQueryID(const int id); int findUpstreamID(const char * upstream, const in_port_t port); int findDomainID(const char *domain, const bool count); int findClientID(const char *client, const bool count, const bool aliasclient); -int findCacheID(int domainID, int clientID, enum query_types query_type); +#define findCacheID(domainID, clientID, query_type) _findCacheID(domainID, clientID, query_type, __FUNCTION__, __LINE__, __FILE__) +int _findCacheID(int domainID, int clientID, enum query_types query_type, const char *func, const int line, const char *file); bool isValidIPv4(const char *addr); bool isValidIPv6(const char *addr); bool is_blocked(const enum query_status status) __attribute__ ((const)); -#define query_set_status(query, new_status) _query_set_status(query, new_status, __FILE__, __LINE__) -void _query_set_status(queriesData *query, const enum query_status new_status, const char *file, const int line); +#define query_set_status(query, new_status) _query_set_status(query, new_status, __FUNCTION__, __LINE__, __FILE__) +void _query_set_status(queriesData *query, const enum query_status new_status, const char *func, const int line, const char *file); void FTL_reload_all_domainlists(void); void FTL_reset_per_client_domain_data(void); @@ -135,14 +136,14 @@ const char *get_query_reply_str(const enum reply_type query) __attribute__ ((con // Pointer getter functions #define getQuery(queryID, checkMagic) _getQuery(queryID, checkMagic, __LINE__, __FUNCTION__, __FILE__) -queriesData* _getQuery(int queryID, bool checkMagic, int line, const char * function, const char * file); +queriesData* _getQuery(int queryID, bool checkMagic, int line, const char *func, const char *file); #define getClient(clientID, checkMagic) _getClient(clientID, checkMagic, __LINE__, __FUNCTION__, __FILE__) -clientsData* _getClient(int clientID, bool checkMagic, int line, const char * function, const char * file); +clientsData* _getClient(int clientID, bool checkMagic, int line, const char *func, const char *file); #define getDomain(domainID, checkMagic) _getDomain(domainID, checkMagic, __LINE__, __FUNCTION__, __FILE__) -domainsData* _getDomain(int domainID, bool checkMagic, int line, const char * function, const char * file); +domainsData* _getDomain(int domainID, bool checkMagic, int line, const char *func, const char *file); #define getUpstream(upstreamID, checkMagic) _getUpstream(upstreamID, checkMagic, __LINE__, __FUNCTION__, __FILE__) -upstreamsData* _getUpstream(int upstreamID, bool checkMagic, int line, const char * function, const char * file); +upstreamsData* _getUpstream(int upstreamID, bool checkMagic, int line, const char *func, const char *file); #define getDNSCache(cacheID, checkMagic) _getDNSCache(cacheID, checkMagic, __LINE__, __FUNCTION__, __FILE__) -DNSCacheData* _getDNSCache(int cacheID, bool checkMagic, int line, const char * function, const char * file); +DNSCacheData* _getDNSCache(int cacheID, bool checkMagic, int line, const char *func, const char *file); #endif //DATASTRUCTURE_H diff --git a/src/shmem.c b/src/shmem.c index 648b3f44..01d94b7b 100644 --- a/src/shmem.c +++ b/src/shmem.c @@ -999,16 +999,13 @@ void set_per_client_regex(const int clientID, const int regexID, const bool valu ((bool*) shm_per_client_regex.ptr)[id] = value; } -static inline bool check_range(int ID, int MAXID, const char* type, int line, const char * function, const char * file) +static inline bool check_range(int ID, int MAXID, const char* type, const char *func, int line, const char *file) { // Check bounds if(ID < 0 || ID > MAXID) { - if(config.debug) - { - logg("ERROR: Trying to access %s ID %i, but maximum is %i", type, ID, MAXID); - logg(" found in %s() (%s:%i)", function, file, line); - } + logg("ERROR: Trying to access %s ID %i, but maximum is %i", type, ID, MAXID); + logg(" found in %s() (%s:%i)", func, short_path(file), line); return false; } @@ -1016,16 +1013,13 @@ static inline bool check_range(int ID, int MAXID, const char* type, int line, co return true; } -static inline bool check_magic(int ID, bool checkMagic, unsigned char magic, const char* type, int line, const char * function, const char * file) +static inline bool check_magic(int ID, bool checkMagic, unsigned char magic, const char *type, const char *func, int line, const char *file) { // Check magic only if requested (skipped for new entries which are uninitialized) if(checkMagic && magic != MAGICBYTE) { - if(config.debug) - { - logg("ERROR: Trying to access %s ID %i, but magic byte is %x", type, ID, magic); - logg(" found in %s() (%s:%i)", function, file, line); - } + logg("ERROR: Trying to access %s ID %i, but magic byte is %x", type, ID, magic); + logg(" found in %s() (%s:%i)", func, short_path(file), line); return false; } @@ -1033,7 +1027,7 @@ static inline bool check_magic(int ID, bool checkMagic, unsigned char magic, con return true; } -queriesData* _getQuery(int queryID, bool checkMagic, int line, const char * function, const char * file) +queriesData* _getQuery(int queryID, bool checkMagic, int line, const char *func, const char *file) { // This does not exist, return a NULL pointer if(queryID == -1) @@ -1043,19 +1037,19 @@ queriesData* _getQuery(int queryID, bool checkMagic, int line, const char * func if(config.debug & DEBUG_LOCKS && !is_our_lock()) { logg("ERROR: Tried to obtain query pointer without lock in %s() (%s:%i)!", - function, file, line); + func, short_path(file), line); generate_backtrace(); return NULL; } - if(check_range(queryID, counters->queries_MAX, "query", line, function, file) && - check_magic(queryID, checkMagic, queries[queryID].magic, "query", line, function, file)) + if(check_range(queryID, counters->queries_MAX, "query", func, line, file) && + check_magic(queryID, checkMagic, queries[queryID].magic, "query", func, line, file)) return &queries[queryID]; else return NULL; } -clientsData* _getClient(int clientID, bool checkMagic, int line, const char * function, const char * file) +clientsData* _getClient(int clientID, bool checkMagic, int line, const char *func, const char *file) { // This does not exist, we return a NULL pointer if(clientID == -1) @@ -1065,19 +1059,19 @@ clientsData* _getClient(int clientID, bool checkMagic, int line, const char * fu if(config.debug & DEBUG_LOCKS && !is_our_lock()) { logg("ERROR: Tried to obtain client pointer without lock in %s() (%s:%i)!", - function, file, line); + func, short_path(file), line); generate_backtrace(); return NULL; } - if(check_range(clientID, counters->clients_MAX, "client", line, function, file) && - check_magic(clientID, checkMagic, clients[clientID].magic, "client", line, function, file)) + if(check_range(clientID, counters->clients_MAX, "client", func, line, file) && + check_magic(clientID, checkMagic, clients[clientID].magic, "client", func, line, file)) return &clients[clientID]; else return NULL; } -domainsData* _getDomain(int domainID, bool checkMagic, int line, const char * function, const char * file) +domainsData* _getDomain(int domainID, bool checkMagic, int line, const char *func, const char *file) { // This does not exist, we return a NULL pointer if(domainID == -1) @@ -1087,19 +1081,19 @@ domainsData* _getDomain(int domainID, bool checkMagic, int line, const char * fu if(config.debug & DEBUG_LOCKS && !is_our_lock()) { logg("ERROR: Tried to obtain domain pointer without lock in %s() (%s:%i)!", - function, file, line); + func, short_path(file), line); generate_backtrace(); return NULL; } - if(check_range(domainID, counters->domains_MAX, "domain", line, function, file) && - check_magic(domainID, checkMagic, domains[domainID].magic, "domain", line, function, file)) + if(check_range(domainID, counters->domains_MAX, "domain", func, line, file) && + check_magic(domainID, checkMagic, domains[domainID].magic, "domain", func, line, file)) return &domains[domainID]; else return NULL; } -upstreamsData* _getUpstream(int upstreamID, bool checkMagic, int line, const char * function, const char * file) +upstreamsData* _getUpstream(int upstreamID, bool checkMagic, int line, const char *func, const char *file) { // This does not exist, we return a NULL pointer if(upstreamID == -1) @@ -1109,19 +1103,19 @@ upstreamsData* _getUpstream(int upstreamID, bool checkMagic, int line, const cha if(config.debug & DEBUG_LOCKS && !is_our_lock()) { logg("ERROR: Tried to obtain upstream pointer without lock in %s() (%s:%i)!", - function, file, line); + func, short_path(file), line); generate_backtrace(); return NULL; } - if(check_range(upstreamID, counters->upstreams_MAX, "upstream", line, function, file) && - check_magic(upstreamID, checkMagic, upstreams[upstreamID].magic, "upstream", line, function, file)) + if(check_range(upstreamID, counters->upstreams_MAX, "upstream", func, line, file) && + check_magic(upstreamID, checkMagic, upstreams[upstreamID].magic, "upstream", func, line, file)) return &upstreams[upstreamID]; else return NULL; } -DNSCacheData* _getDNSCache(int cacheID, bool checkMagic, int line, const char * function, const char * file) +DNSCacheData* _getDNSCache(int cacheID, bool checkMagic, int line, const char *func, const char *file) { // This does not exist, we return a NULL pointer if(cacheID == -1) @@ -1131,13 +1125,13 @@ DNSCacheData* _getDNSCache(int cacheID, bool checkMagic, int line, const char * if(config.debug & DEBUG_LOCKS && !is_our_lock()) { logg("ERROR: Tried to obtain cache pointer without lock in %s() (%s:%i)!", - function, file, line); + func, short_path(file), line); generate_backtrace(); return NULL; } - if(check_range(cacheID, counters->dns_cache_MAX, "dns_cache", line, function, file) && - check_magic(cacheID, checkMagic, dns_cache[cacheID].magic, "dns_cache", line, function, file)) + if(check_range(cacheID, counters->dns_cache_MAX, "dns_cache", func, line, file) && + check_magic(cacheID, checkMagic, dns_cache[cacheID].magic, "dns_cache", func, line, file)) return &dns_cache[cacheID]; else return NULL; From 1ccfd9a081490a2ae5ca84321c1ec0af09cfffa2 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 20 Aug 2022 15:27:38 +0200 Subject: [PATCH 4/5] Ensure the API does not try to create DNS cache records for queries that don't have one Signed-off-by: DL6ER --- src/api/api.c | 2 +- src/database/query-table.c | 4 ++-- src/datastructure.c | 5 ++++- src/datastructure.h | 4 ++-- src/dnsmasq_interface.c | 6 +++--- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/api/api.c b/src/api/api.c index 67da3334..441f1626 100644 --- a/src/api/api.c +++ b/src/api/api.c @@ -1012,7 +1012,7 @@ void getAllQueries(const char *client_message, const int sock, const bool isteln int domainlist_id = -1; if (config.privacylevel < PRIVACY_HIDE_DOMAINS) { - unsigned int cacheID = findCacheID(query->domainID, query->clientID, query->type); + unsigned int cacheID = findCacheID(query->domainID, query->clientID, query->type, false); DNSCacheData *dns_cache = getDNSCache(cacheID, true); if(dns_cache != NULL) domainlist_id = dns_cache->domainlist_id; diff --git a/src/database/query-table.c b/src/database/query-table.c index 3b50b66f..ed532066 100644 --- a/src/database/query-table.c +++ b/src/database/query-table.c @@ -373,7 +373,7 @@ int DB_save_queries(sqlite3 *db) sqlite3_bind_null(query_stmt, 7); } - const int cacheID = findCacheID(query->domainID, query->clientID, query->type); + const int cacheID = findCacheID(query->domainID, query->clientID, query->type, false); DNSCacheData *cache = getDNSCache(cacheID, true); // ADDITIONAL_INFO @@ -933,7 +933,7 @@ void DB_read_queries(void) // Set ID of the domainlist entry that was the reason for permitting/blocking this query // We assume the value in this field is said ID when it is not a CNAME-related domain // (checked above) and the value of additional_info is not NULL (0 bytes storage size) - const int cacheID = findCacheID(query->domainID, query->clientID, query->type); + const int cacheID = findCacheID(query->domainID, query->clientID, query->type, true); DNSCacheData *cache = getDNSCache(cacheID, true); // Only load if // a) we have a cache entry diff --git a/src/datastructure.c b/src/datastructure.c index e1aa775a..904e49ca 100644 --- a/src/datastructure.c +++ b/src/datastructure.c @@ -319,7 +319,7 @@ void change_clientcount(clientsData *client, int total, int blocked, int overTim } } -int _findCacheID(int domainID, int clientID, enum query_types query_type, const char *func, int line, const char *file) +int _findCacheID(const int domainID, const int clientID, const enum query_types query_type, const bool create_new, const char *func, int line, const char *file) { // Compare content of client against known client IP addresses for(int cacheID = 0; cacheID < counters->dns_cache_size; cacheID++) @@ -339,6 +339,9 @@ int _findCacheID(int domainID, int clientID, enum query_types query_type, const } } + if(!create_new) + return -1; + // Get ID of new cache entry const int cacheID = counters->dns_cache_size; diff --git a/src/datastructure.h b/src/datastructure.h index e1ef2779..a37db29c 100644 --- a/src/datastructure.h +++ b/src/datastructure.h @@ -113,8 +113,8 @@ int findQueryID(const int id); int findUpstreamID(const char * upstream, const in_port_t port); int findDomainID(const char *domain, const bool count); int findClientID(const char *client, const bool count, const bool aliasclient); -#define findCacheID(domainID, clientID, query_type) _findCacheID(domainID, clientID, query_type, __FUNCTION__, __LINE__, __FILE__) -int _findCacheID(int domainID, int clientID, enum query_types query_type, const char *func, const int line, const char *file); +#define findCacheID(domainID, clientID, query_type, create_new) _findCacheID(domainID, clientID, query_type, create_new, __FUNCTION__, __LINE__, __FILE__) +int _findCacheID(const int domainID, const int clientID, const enum query_types query_type, const bool create_new, const char *func, const int line, const char *file); bool isValidIPv4(const char *addr); bool isValidIPv6(const char *addr); diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c index 4c31c9f2..a652c44a 100644 --- a/src/dnsmasq_interface.c +++ b/src/dnsmasq_interface.c @@ -1249,7 +1249,7 @@ static bool _FTL_check_blocking(int queryID, int domainID, int clientID, const c } // Get cache pointer - unsigned int cacheID = findCacheID(domainID, clientID, query->type); + unsigned int cacheID = findCacheID(domainID, clientID, query->type, true); DNSCacheData *dns_cache = getDNSCache(cacheID, true); if(dns_cache == NULL) { @@ -1581,8 +1581,8 @@ bool _FTL_CNAME(const char *domain, const struct crec *cpp, const int id, const else if(query->status == QUERY_REGEX) { // Get parent and child DNS cache entries - const int parent_cacheID = findCacheID(parent_domainID, clientID, query->type); - const int child_cacheID = findCacheID(child_domainID, clientID, query->type); + const int parent_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); From bea963088283631eb484973314e462e23b1029b8 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sun, 11 Sep 2022 11:31:08 +0200 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: yubiuser Signed-off-by: DL6ER --- src/api/api.c | 2 +- src/database/gravity-db.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/api/api.c b/src/api/api.c index 441f1626..2e807f33 100644 --- a/src/api/api.c +++ b/src/api/api.c @@ -1008,7 +1008,7 @@ void getAllQueries(const char *client_message, const int sock, const bool isteln CNAME_domain = getCNAMEDomainString(query); } - // Get ID of blocking regex, if applicable and permitted by privacy settings + // Get domainlist table ID, if applicable and permitted by privacy settings int domainlist_id = -1; if (config.privacylevel < PRIVACY_HIDE_DOMAINS) { diff --git a/src/database/gravity-db.c b/src/database/gravity-db.c index d4c5d6d0..f242995e 100644 --- a/src/database/gravity-db.c +++ b/src/database/gravity-db.c @@ -1204,7 +1204,6 @@ static enum db_result domain_in_list(const char *domain, sqlite3_stmt *stmt, con sqlite3_clear_bindings(stmt); // Return if domain was found in current table - // SELECT EXISTS(...) either returns 0 (false) or 1 (true). return (rc == SQLITE_ROW) ? FOUND : NOT_FOUND; }