From 333163529f1c169fbd7b8f5239ee436c666dc407 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 3 Feb 2018 12:20:38 +0100 Subject: [PATCH 1/8] Fix signedness of integer constant. This was a problem on 32bit platforms. Signed-off-by: DL6ER --- msgpack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msgpack.c b/msgpack.c index 0fe622de..0f512234 100644 --- a/msgpack.c +++ b/msgpack.c @@ -96,7 +96,7 @@ bool pack_str32(int sock, char *string) { // Make sure that the length is less than 4294967296 size_t length = strlen(string); - if(length >= 2147483648) { + if(length >= 2147483648u) { logg("Tried to send a str32 longer than 2147483647 bytes!"); return false; } From 4e056dfd7e837853c4209ba7f40592a57b371a5b Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 5 Feb 2018 17:06:37 +0100 Subject: [PATCH 2/8] Always store client IP in the database (never the host name). We do this for consistency as we will always have IP addresses but not always host names so the current situation can actually be messy in the database. Furthermore, it should be noted that we also only ever store IP addresses (and never host names!) for the forward destinations. Signed-off-by: DL6ER --- database.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/database.c b/database.c index 58f930ec..b6237741 100644 --- a/database.c +++ b/database.c @@ -354,10 +354,7 @@ void save_to_DB(void) sqlite3_bind_text(stmt, 4, domains[queries[i].domainID].domain, -1, SQLITE_TRANSIENT); // CLIENT - if(strlen(clients[queries[i].clientID].name) > 0) - sqlite3_bind_text(stmt, 5, clients[queries[i].clientID].name, -1, SQLITE_TRANSIENT); - else - sqlite3_bind_text(stmt, 5, clients[queries[i].clientID].ip, -1, SQLITE_TRANSIENT); + sqlite3_bind_text(stmt, 5, clients[queries[i].clientID].ip, -1, SQLITE_TRANSIENT); // FORWARD if(queries[i].status == 2 && queries[i].forwardID > -1) From ecde321632a2fcd0ecb23bab74731fa83c92d9d2 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Wed, 7 Feb 2018 00:24:12 +0100 Subject: [PATCH 3/8] Add custom macros for the memory-functions - calloc, - realloc, - free, and - strdup These functions will not directly improve memory handling but they will log if something will go wrong *before* it actually goes wrong. This will ease debugging when user provide log snippets especially in ENOMEM (no memory available) situations. Signed-off-by: DL6ER --- FTL.h | 11 ++ Makefile | 2 +- memory.c | 290 +++++++++++++++++++++++++++++++++++++++++++++++++++++ parser.c | 90 ----------------- routines.h | 12 ++- structs.c | 126 ----------------------- 6 files changed, 311 insertions(+), 220 deletions(-) create mode 100644 memory.c delete mode 100644 structs.c diff --git a/FTL.h b/FTL.h index e7aaaab8..220549d7 100644 --- a/FTL.h +++ b/FTL.h @@ -254,3 +254,14 @@ bool rereadgravity; long int lastDBimportedtimestamp; bool ipv4telnet, ipv6telnet; bool istelnet[MAXCONNS]; + +// Use out own memory handling functions that will detect possible errors +// and report accordingly in the log. This will make debugging FTL crashs +// caused by insufficient memory or by code bugs (not properly dealing +// with NULL pointers) much easier. +#define free(param) FTLfree(param, __FILE__, __FUNCTION__, __LINE__) +#define lib_strdup() strdup() +#undef strdup +#define strdup(param) FTLstrdup(param, __FILE__, __FUNCTION__, __LINE__) +#define calloc(p1,p2) FTLcalloc(p1,p2, __FILE__, __FUNCTION__, __LINE__) +#define realloc(p1,p2) FTLrealloc(p1,p2, __FILE__, __FUNCTION__, __LINE__) diff --git a/Makefile b/Makefile index e662a53c..8250d043 100644 --- a/Makefile +++ b/Makefile @@ -9,7 +9,7 @@ # Please see LICENSE file for your rights under this license. DEPS = FTL.h routines.h api.h version.h -OBJ = main.o structs.o log.o daemon.o parser.o signals.o socket.o request.o grep.o setupVars.o args.o flush.o threads.o gc.o config.o database.o api.o msgpack.o +OBJ = main.o memory.o log.o daemon.o parser.o signals.o socket.o request.o grep.o setupVars.o args.o flush.o threads.o gc.o config.o database.o api.o msgpack.o # Get git commit version and date GIT_BRANCH := $(shell git branch | sed -n 's/^\* //p') diff --git a/memory.c b/memory.c new file mode 100644 index 00000000..857d39d6 --- /dev/null +++ b/memory.c @@ -0,0 +1,290 @@ +/* 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 +* Global variable definitions and memory reallocation handling +* +* This file is copyright under the latest version of the EUPL. +* Please see LICENSE file for your rights under this license. */ + +#include "FTL.h" + +FTLFileNamesStruct FTLfiles = { + "/etc/pihole/pihole-FTL.conf", + "/var/log/pihole-FTL.log", + "/var/run/pihole-FTL.pid", + "/var/run/pihole-FTL.port", + NULL, + "/var/run/pihole/FTL.sock" +}; + +logFileNamesStruct files = { + "/var/log/pihole.log", + "/etc/pihole/list.preEventHorizon", + "/etc/pihole/whitelist.txt", + "/etc/pihole/blacklist.txt", + "/etc/pihole/setupVars.conf", + "/etc/dnsmasq.d/03-pihole-wildcard.conf", + "/etc/pihole/auditlog.list", + "/etc/dnsmasq.d/01-pihole.conf" +}; + +countersStruct counters = { 0 }; + +void memory_check(int which) +{ + switch(which) + { + case QUERIES: + if(counters.queries >= counters.queries_MAX) + { + // Have to reallocate memory + counters.queries_MAX += QUERIESALLOCSTEP; + logg_struct_resize("queries",counters.queries_MAX,QUERIESALLOCSTEP); + queries = realloc(queries, counters.queries_MAX*sizeof(queriesDataStruct)); + if(queries == NULL) + { + logg("FATAL: Memory allocation failed! Exiting"); + exit(EXIT_FAILURE); + } + } + break; + case FORWARDED: + if(counters.forwarded >= counters.forwarded_MAX) + { + // Have to reallocate memory + counters.forwarded_MAX += FORWARDEDALLOCSTEP; + logg_struct_resize("forwarded",counters.forwarded_MAX,FORWARDEDALLOCSTEP); + forwarded = realloc(forwarded, counters.forwarded_MAX*sizeof(forwardedDataStruct)); + if(forwarded == NULL) + { + logg("FATAL: Memory allocation failed! Exiting"); + exit(EXIT_FAILURE); + } + } + break; + case CLIENTS: + if(counters.clients >= counters.clients_MAX) + { + // Have to reallocate memory + counters.clients_MAX += CLIENTSALLOCSTEP; + logg_struct_resize("clients",counters.clients_MAX,CLIENTSALLOCSTEP); + clients = realloc(clients, counters.clients_MAX*sizeof(clientsDataStruct)); + if(clients == NULL) + { + logg("FATAL: Memory allocation failed! Exiting"); + exit(EXIT_FAILURE); + } + } + break; + case DOMAINS: + if(counters.domains >= counters.domains_MAX) + { + // Have to reallocate memory + counters.domains_MAX += DOMAINSALLOCSTEP; + logg_struct_resize("domains",counters.domains_MAX,DOMAINSALLOCSTEP); + domains = realloc(domains, counters.domains_MAX*sizeof(domainsDataStruct)); + if(domains == NULL) + { + logg("FATAL: Memory allocation failed! Exiting"); + exit(EXIT_FAILURE); + } + } + break; + case OVERTIME: + if(counters.overTime >= counters.overTime_MAX) + { + // Have to reallocate memory + counters.overTime_MAX += OVERTIMEALLOCSTEP; + logg_struct_resize("overTime",counters.overTime_MAX,OVERTIMEALLOCSTEP); + overTime = realloc(overTime, counters.overTime_MAX*sizeof(overTimeDataStruct)); + if(overTime == NULL) + { + logg("FATAL: Memory allocation failed! Exiting"); + exit(EXIT_FAILURE); + } + } + break; + case WILDCARD: + // Definitely enlarge wildcard entry + // Enlarge wildcarddomains pointer array + logg_struct_resize("wildcards", (counters.wildcarddomains+1), 1); + wildcarddomains = realloc(wildcarddomains, (counters.wildcarddomains+1)*sizeof(*wildcarddomains)); + if(wildcarddomains == NULL) + { + logg("FATAL: Memory allocation failed! Exiting"); + exit(EXIT_FAILURE); + } + break; + default: + /* That cannot happen */ + logg("Fatal error in memory_check(%i)", which); + exit(EXIT_FAILURE); + break; + } +} + +void validate_access(const char * name, int pos, bool testmagic, int line, const char * function, const char * file) +{ + int limit = 0; + if(name[0] == 'c') limit = counters.clients_MAX; + else if(name[0] == 'd') limit = counters.domains_MAX; + else if(name[0] == 'q') limit = counters.queries_MAX; + else if(name[0] == 'o') limit = counters.overTime_MAX; + else if(name[0] == 'f') limit = counters.forwarded_MAX; + else if(name[0] == 'w') limit = counters.wildcarddomains; + else { logg("Validator error (range)"); killed = 1; } + + if(pos >= limit || pos < 0) + { + logg("FATAL ERROR: Trying to access %s[%i], but maximum is %i", name, pos, limit); + logg(" found in %s() (%s:%i)", function, file, line); + } + // Don't test magic byte if detected potential out-of-bounds error + else if(testmagic) + { + unsigned char magic = 0x00; + if(name[0] == 'c') magic = clients[pos].magic; + else if(name[0] == 'd') magic = domains[pos].magic; + else if(name[0] == 'q') magic = queries[pos].magic; + else if(name[0] == 'o') magic = overTime[pos].magic; + else if(name[0] == 'f') magic = forwarded[pos].magic; + else { logg("Validator error (magic byte)"); killed = 1; } + if(magic != MAGICBYTE) + { + logg("FATAL ERROR: Trying to access %s[%i], but magic byte is %x", name, pos, magic); + logg(" found in %s() (%s:%i)", function, file, line); + } + } +} + +void validate_access_oTfd(int timeidx, int forwardID, int line, const char * function, const char * file) +{ + // Determine if there is enough space for saving the current + // forwardID in the overTime data structure, allocate space otherwise + validate_access("overTime", timeidx, true, __LINE__, __FUNCTION__, __FILE__); + if(overTime[timeidx].forwardnum <= forwardID) + { + // Reallocate more space for forwarddata + overTime[timeidx].forwarddata = realloc(overTime[timeidx].forwarddata, (forwardID+1)*sizeof(*overTime[timeidx].forwarddata)); + // Initialize new data fields with zeroes + int j; + for(j = overTime[timeidx].forwardnum; j <= forwardID; j++) + { + overTime[timeidx].forwarddata[j] = 0; + memory.forwarddata++; + } + // Update counter + overTime[timeidx].forwardnum = forwardID + 1; + } + + int limit = overTime[timeidx].forwardnum; + if(forwardID >= limit || forwardID < 0) + { + logg("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + logg("FATAL ERROR: Trying to access overTime.forwarddata[%i], but maximum is %i", forwardID, limit); + logg(" found in %s() (%s:%i)", function, file, line); + } +} + +void validate_access_oTcl(int timeidx, int clientID, int line, const char * function, const char * file) +{ + // Determine if there is enough space for saving the current + // clientID in the overTime data structure, allocate space otherwise + if(overTime[timeidx].clientnum <= clientID) + { + // Reallocate more space for clientdata + overTime[timeidx].clientdata = realloc(overTime[timeidx].clientdata, (clientID+1)*sizeof(*overTime[timeidx].clientdata)); + // Initialize new data fields with zeroes + int i; + for(i = overTime[timeidx].clientnum; i <= clientID; i++) + { + overTime[timeidx].clientdata[i] = 0; + memory.clientdata++; + } + // Update counter + overTime[timeidx].clientnum = clientID + 1; + } + int limit = overTime[timeidx].clientnum; + if(clientID >= limit || clientID < 0) + { + logg("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + logg("FATAL ERROR: Trying to access overTime.clientdata[%i], but maximum is %i", clientID, limit); + logg(" found in %s() (%s:%i)", function, file, line); + } +} + +// The special memory handling routines have to be the last ones in this source file +// as we restore the original definition of the strdup, free, calloc, and realloc +// functions in here, i.e. if anything extra would come below these lines, it would +// not be protected by our (error logging) functions! + +#undef strdup +char *FTLstrdup(const char *src, const char * file, const char * function, int line) +{ + // The FTLstrdup() function returns a pointer to a new string which is a + // duplicate of the string s. Memory for the new string is obtained with + // malloc(3), and can be freed with free(3). + if(src == NULL) + { + logg("WARN: Trying to copy a NULL string in %s() (%s:%i)", function, file, line); + return NULL; + } + char *dest = __strdup(src); + if(dest == NULL) + logg("FATAL: Memory allocation failed in %s() (%s:%i)", function, file, line); + + return dest; +} + +#undef calloc +void *FTLcalloc(size_t nmemb, size_t size, const char * file, const char * function, int line) +{ + // The FTLcalloc() function allocates memory for an array of nmemb elements + // of size bytes each and returns a pointer to the allocated memory. The + // memory is set to zero. If nmemb or size is 0, then calloc() returns + // either NULL, or a unique pointer value that can later be successfully + // passed to free(). + void *ptr = calloc(nmemb, size); + if(ptr == NULL) + logg("FATAL: Memory allocation (%u x %u) failed in %s() (%s:%i)", + nmemb, size, function, file, line); + + return ptr; +} + +#undef realloc +void *FTLrealloc(void *ptr_in, size_t size, const char * file, const char * function, int line) +{ + // The FTLrealloc() function changes the size of the memory block pointed to + // by ptr to size bytes. The contents will be unchanged in the range from + // the start of the region up to the minimum of the old and new sizes. If + // the new size is larger than the old size, the added memory will not be + // initialized. If ptr is NULL, then the call is equivalent to malloc(size), + // for all values of size; if size is equal to zero, and ptr is + // not NULL, then the call is equivalent to free(ptr). Unless ptr is + // NULL, it must have been returned by an earlier call to malloc(), cal‐ + // loc() or realloc(). If the area pointed to was moved, a free(ptr) is + // done. + void *ptr_out = realloc(ptr_in, size); + if(ptr_out == NULL) + logg("FATAL: Memory reallocation (%p -> %u) failed in %s() (%s:%i)", + ptr_in, size, function, file, line); + + return ptr_out; +} + +#undef free +void FTLfree(void *ptr, const char * file, const char * function, int line) +{ + // The free() function frees the memory space pointed to by ptr, which + // must have been returned by a previous call to malloc(), calloc(), or + // realloc(). Otherwise, or if free(ptr) has already been called before, + // undefined behavior occurs. If ptr is NULL, no operation is performed. + if(ptr == NULL) + logg("FATAL: Trying to free NULL pointer in %s() (%s:%i)", function, file, line); + + // We intentionally run free() nevertheless to see the crash in the debugger + free(ptr); +} diff --git a/parser.c b/parser.c index abad8dc7..d66483a1 100644 --- a/parser.c +++ b/parser.c @@ -1282,96 +1282,6 @@ int findClientID(const char *client) return clientID; } -void validate_access(const char * name, int pos, bool testmagic, int line, const char * function, const char * file) -{ - int limit = 0; - if(name[0] == 'c') limit = counters.clients_MAX; - else if(name[0] == 'd') limit = counters.domains_MAX; - else if(name[0] == 'q') limit = counters.queries_MAX; - else if(name[0] == 'o') limit = counters.overTime_MAX; - else if(name[0] == 'f') limit = counters.forwarded_MAX; - else if(name[0] == 'w') limit = counters.wildcarddomains; - else { logg("Validator error (range)"); killed = 1; } - - if(pos >= limit || pos < 0) - { - logg("FATAL ERROR: Trying to access %s[%i], but maximum is %i", name, pos, limit); - logg(" found in %s() (line %i) in %s", function, line, file); - } - // Don't test magic byte if detected potential out-of-bounds error - else if(testmagic) - { - unsigned char magic = 0x00; - if(name[0] == 'c') magic = clients[pos].magic; - else if(name[0] == 'd') magic = domains[pos].magic; - else if(name[0] == 'q') magic = queries[pos].magic; - else if(name[0] == 'o') magic = overTime[pos].magic; - else if(name[0] == 'f') magic = forwarded[pos].magic; - else { logg("Validator error (magic byte)"); killed = 1; } - if(magic != MAGICBYTE) - { - logg("FATAL ERROR: Trying to access %s[%i], but magic byte is %x", name, pos, magic); - logg(" found in %s() (line %i) in %s", function, line, file); - } - } -} - -void validate_access_oTfd(int timeidx, int forwardID, int line, const char * function, const char * file) -{ - // Determine if there is enough space for saving the current - // forwardID in the overTime data structure, allocate space otherwise - validate_access("overTime", timeidx, true, __LINE__, __FUNCTION__, __FILE__); - if(overTime[timeidx].forwardnum <= forwardID) - { - // Reallocate more space for forwarddata - overTime[timeidx].forwarddata = realloc(overTime[timeidx].forwarddata, (forwardID+1)*sizeof(*overTime[timeidx].forwarddata)); - // Initialize new data fields with zeroes - int j; - for(j = overTime[timeidx].forwardnum; j <= forwardID; j++) - { - overTime[timeidx].forwarddata[j] = 0; - memory.forwarddata++; - } - // Update counter - overTime[timeidx].forwardnum = forwardID + 1; - } - - int limit = overTime[timeidx].forwardnum; - if(forwardID >= limit || forwardID < 0) - { - logg("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); - logg("FATAL ERROR: Trying to access overTime.forwarddata[%i], but maximum is %i", forwardID, limit); - logg(" found in %s() (line %i) in %s", function, line, file); - } -} - -void validate_access_oTcl(int timeidx, int clientID, int line, const char * function, const char * file) -{ - // Determine if there is enough space for saving the current - // clientID in the overTime data structure, allocate space otherwise - if(overTime[timeidx].clientnum <= clientID) - { - // Reallocate more space for clientdata - overTime[timeidx].clientdata = realloc(overTime[timeidx].clientdata, (clientID+1)*sizeof(*overTime[timeidx].clientdata)); - // Initialize new data fields with zeroes - int i; - for(i = overTime[timeidx].clientnum; i <= clientID; i++) - { - overTime[timeidx].clientdata[i] = 0; - memory.clientdata++; - } - // Update counter - overTime[timeidx].clientnum = clientID + 1; - } - int limit = overTime[timeidx].clientnum; - if(clientID >= limit || clientID < 0) - { - logg("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); - logg("FATAL ERROR: Trying to access overTime.clientdata[%i], but maximum is %i", clientID, limit); - logg(" found in %s() (line %i) in %s", function, line, file); - } -} - void reresolveHostnames(void) { int clientID; diff --git a/routines.h b/routines.h index ae59f9dc..b31fb686 100644 --- a/routines.h +++ b/routines.h @@ -29,9 +29,6 @@ void open_pihole_log(void); void handle_signals(void); void process_pihole_log(void); void *pihole_log_thread(void *val); -void validate_access(const char * name, int pos, bool testmagic, int line, const char * function, const char * file); -void validate_access_oTfd(int timeidx, int forwardID, int line, const char * function, const char * file); -void validate_access_oTcl(int timeidx, int clientID, int line, const char * function, const char * file); void reresolveHostnames(void); int findClientID(const char *client); int findDomainID(const char *domain); @@ -88,3 +85,12 @@ void *DB_thread(void *val); int get_number_of_queries_in_DB(void); void save_to_DB(void); void read_data_from_DB(void); + +// memory.c +char *FTLstrdup(const char *src, const char *file, const char *function, int line); +void *FTLcalloc(size_t nmemb, size_t size, const char *file, const char *function, int line); +void *FTLrealloc(void *ptr_in, size_t size, const char *file, const char *function, int line); +void FTLfree(void *ptr, const char* file, const char *function, int line); +void validate_access(const char * name, int pos, bool testmagic, int line, const char * function, const char * file); +void validate_access_oTfd(int timeidx, int forwardID, int line, const char * function, const char * file); +void validate_access_oTcl(int timeidx, int clientID, int line, const char * function, const char * file); diff --git a/structs.c b/structs.c deleted file mode 100644 index b49228fb..00000000 --- a/structs.c +++ /dev/null @@ -1,126 +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 -* Global variable definitions and memory reallocation handling -* -* This file is copyright under the latest version of the EUPL. -* Please see LICENSE file for your rights under this license. */ - -#include "FTL.h" - -FTLFileNamesStruct FTLfiles = { - "/etc/pihole/pihole-FTL.conf", - "/var/log/pihole-FTL.log", - "/var/run/pihole-FTL.pid", - "/var/run/pihole-FTL.port", - NULL, - "/var/run/pihole/FTL.sock" -}; - -logFileNamesStruct files = { - "/var/log/pihole.log", - "/etc/pihole/list.preEventHorizon", - "/etc/pihole/whitelist.txt", - "/etc/pihole/blacklist.txt", - "/etc/pihole/setupVars.conf", - "/etc/dnsmasq.d/03-pihole-wildcard.conf", - "/etc/pihole/auditlog.list", - "/etc/dnsmasq.d/01-pihole.conf" -}; - -countersStruct counters = { 0 }; - -void memory_check(int which) -{ - switch(which) - { - case QUERIES: - if(counters.queries >= counters.queries_MAX) - { - // Have to reallocate memory - counters.queries_MAX += QUERIESALLOCSTEP; - logg_struct_resize("queries",counters.queries_MAX,QUERIESALLOCSTEP); - queries = realloc(queries, counters.queries_MAX*sizeof(queriesDataStruct)); - if(queries == NULL) - { - logg("FATAL: Memory allocation failed! Exiting"); - exit(EXIT_FAILURE); - } - } - break; - case FORWARDED: - if(counters.forwarded >= counters.forwarded_MAX) - { - // Have to reallocate memory - counters.forwarded_MAX += FORWARDEDALLOCSTEP; - logg_struct_resize("forwarded",counters.forwarded_MAX,FORWARDEDALLOCSTEP); - forwarded = realloc(forwarded, counters.forwarded_MAX*sizeof(forwardedDataStruct)); - if(forwarded == NULL) - { - logg("FATAL: Memory allocation failed! Exiting"); - exit(EXIT_FAILURE); - } - } - break; - case CLIENTS: - if(counters.clients >= counters.clients_MAX) - { - // Have to reallocate memory - counters.clients_MAX += CLIENTSALLOCSTEP; - logg_struct_resize("clients",counters.clients_MAX,CLIENTSALLOCSTEP); - clients = realloc(clients, counters.clients_MAX*sizeof(clientsDataStruct)); - if(clients == NULL) - { - logg("FATAL: Memory allocation failed! Exiting"); - exit(EXIT_FAILURE); - } - } - break; - case DOMAINS: - if(counters.domains >= counters.domains_MAX) - { - // Have to reallocate memory - counters.domains_MAX += DOMAINSALLOCSTEP; - logg_struct_resize("domains",counters.domains_MAX,DOMAINSALLOCSTEP); - domains = realloc(domains, counters.domains_MAX*sizeof(domainsDataStruct)); - if(domains == NULL) - { - logg("FATAL: Memory allocation failed! Exiting"); - exit(EXIT_FAILURE); - } - } - break; - case OVERTIME: - if(counters.overTime >= counters.overTime_MAX) - { - // Have to reallocate memory - counters.overTime_MAX += OVERTIMEALLOCSTEP; - logg_struct_resize("overTime",counters.overTime_MAX,OVERTIMEALLOCSTEP); - overTime = realloc(overTime, counters.overTime_MAX*sizeof(overTimeDataStruct)); - if(overTime == NULL) - { - logg("FATAL: Memory allocation failed! Exiting"); - exit(EXIT_FAILURE); - } - } - break; - case WILDCARD: - // Definitely enlarge wildcard entry - // Enlarge wildcarddomains pointer array - logg_struct_resize("wildcards", (counters.wildcarddomains+1), 1); - wildcarddomains = realloc(wildcarddomains, (counters.wildcarddomains+1)*sizeof(*wildcarddomains)); - if(wildcarddomains == NULL) - { - logg("FATAL: Memory allocation failed! Exiting"); - exit(EXIT_FAILURE); - } - break; - default: - /* That cannot happen */ - logg("Fatal error in memory_check(%i)", which); - exit(EXIT_FAILURE); - break; - } -} From 10962a07a65fe94bc27a55fb0a940722879de49b Mon Sep 17 00:00:00 2001 From: DL6ER Date: Wed, 7 Feb 2018 00:29:21 +0100 Subject: [PATCH 4/8] Improve memory handling in api.c Signed-off-by: DL6ER --- api.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/api.c b/api.c index dbc6add3..f4c103c9 100644 --- a/api.c +++ b/api.c @@ -272,6 +272,7 @@ void getTopDomains(char *client_message, int *sock) ssend(*sock, "%i %i %s wildcard\n", n, domains[j].blockedcount, domains[j].domain); else { char *fancyWildcard = calloc(3 + strlen(domains[j].domain), sizeof(char)); + if(fancyWildcard == NULL) return; sprintf(fancyWildcard, "*.%s", domains[j].domain); if(!pack_str32(*sock, fancyWildcard)) @@ -466,10 +467,8 @@ void getForwardDestinations(char *client_message, int *sock) // Is this the "local" forward destination? if(j == counters.forwarded) { - ip = calloc(4,1); - strcpy(ip, "::1"); - name = calloc(6,1); - strcpy(name, "local"); + ip = strdup("::1"); + name = strdup("local"); if(totalqueries > 0) // Whats the percentage of (cached + blocked) queries on the total amount of queries? @@ -582,6 +581,7 @@ void getAllQueries(char *client_message, 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; sscanf(client_message, ">getallqueries-domain %255s", domainname); if(debugclients) logg("Showing only queries with domain %s", domainname); @@ -589,8 +589,9 @@ void getAllQueries(char *client_message, int *sock) } // Client filtering? if(command(client_message, ">getallqueries-client")) { - clientname = calloc(256, sizeof(char)); // Get client name we want to see only (limit length to 255 chars) + clientname = calloc(256, sizeof(char)); + if(clientname == NULL) return; sscanf(client_message, ">getallqueries-client %255s", clientname); if(debugclients) logg("Showing only queries with client %s", clientname); @@ -785,6 +786,7 @@ void getMemoryUsage(int *sock) { unsigned long int structbytes = sizeof(countersStruct) + sizeof(ConfigStruct) + counters.queries_MAX*sizeof(queriesDataStruct) + counters.forwarded_MAX*sizeof(forwardedDataStruct) + counters.clients_MAX*sizeof(clientsDataStruct) + counters.domains_MAX*sizeof(domainsDataStruct) + counters.overTime_MAX*sizeof(overTimeDataStruct) + (counters.wildcarddomains)*sizeof(*wildcarddomains); char *structprefix = calloc(2, sizeof(char)); + if(structprefix == NULL) return; double formated = 0.0; format_memory_size(structprefix, structbytes, &formated); @@ -796,6 +798,7 @@ void getMemoryUsage(int *sock) unsigned long int dynamicbytes = memory.wildcarddomains + memory.domainnames + memory.clientips + memory.clientnames + memory.forwardedips + memory.forwardednames + memory.forwarddata; char *dynamicprefix = calloc(2, sizeof(char)); + if(dynamicprefix == NULL) return; format_memory_size(dynamicprefix, dynamicbytes, &formated); if(istelnet[*sock]) @@ -806,6 +809,7 @@ void getMemoryUsage(int *sock) unsigned long int totalbytes = structbytes + dynamicbytes; char *totalprefix = calloc(2, sizeof(char)); + if(totalprefix == NULL) return; format_memory_size(totalprefix, totalbytes, &formated); if(istelnet[*sock]) @@ -1020,6 +1024,7 @@ void getVersion(int *sock) ssend(*sock, "version vDev-%s\ntag %s\nbranch %s\ndate %s\n", hash, tag, GIT_BRANCH, GIT_DATE); else { char *hashVersion = calloc(6 + strlen(hash), sizeof(char)); + if(hashVersion == NULL) return; sprintf(hashVersion, "vDev-%s", hash); if(!pack_str32(*sock, hashVersion) || @@ -1048,6 +1053,7 @@ void getDBstats(int *sock) filesize = st.st_size; char *prefix = calloc(2, sizeof(char)); + if(prefix == NULL) return; double formated = 0.0; format_memory_size(prefix, filesize, &formated); From e75bd2699444bca7f39d1a455a1e4530c09d3db6 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Wed, 7 Feb 2018 01:23:54 +0100 Subject: [PATCH 5/8] Improve overall memory handling by checking for failed memory allocation events at the places we need the allocated space Signed-off-by: DL6ER --- daemon.c | 7 +++---- grep.c | 1 + log.c | 1 + parser.c | 14 +++++++++----- setupVars.c | 3 +++ socket.c | 11 +++++++---- 6 files changed, 24 insertions(+), 13 deletions(-) diff --git a/daemon.c b/daemon.c index 2fad60c7..e43b2513 100644 --- a/daemon.c +++ b/daemon.c @@ -252,13 +252,12 @@ char *getUserName(void) struct passwd *pw = getpwuid(euid); if(pw) { - username = calloc(strlen(pw->pw_name)+1, sizeof(char)); - strcpy(username, pw->pw_name); + username = strdup(pw->pw_name); } else { - username = calloc(12, sizeof(char)); - sprintf(username, "%i", euid); + if(asprintf(&username, "%i", euid) < 0) + return NULL; } return username; diff --git a/grep.c b/grep.c index bc828836..f8e3cabe 100644 --- a/grep.c +++ b/grep.c @@ -151,6 +151,7 @@ void readWildcardsList() memory_check(WILDCARD); // Allocate space for new domain entry and save domain wildcarddomains[counters.wildcarddomains] = calloc(strlen(domain)+1,sizeof(char)); + if(wildcarddomains[counters.wildcarddomains] == NULL) return; memory.wildcarddomains += (strlen(domain) + 1) * sizeof(char); strcpy(wildcarddomains[counters.wildcarddomains], domain); diff --git a/log.c b/log.c index 4e257fed..ee255136 100644 --- a/log.c +++ b/log.c @@ -120,6 +120,7 @@ void logg_struct_resize(const char* str, int to, int step) unsigned long int bytes = structbytes + dynamicbytes; char *prefix = calloc(2, sizeof(char)); + if(prefix == NULL) return; double formated = 0.0; format_memory_size(prefix, bytes, &formated); diff --git a/parser.c b/parser.c index d66483a1..8fdcc406 100644 --- a/parser.c +++ b/parser.c @@ -328,6 +328,7 @@ void process_pihole_log(void) } char *domain = calloc(domainlen+1,sizeof(char)); + if(domain == NULL) continue; // strncat() NULL-terminates the copied string (strncpy() doesn't!) strncat(domain,domainstart+2,domainlen); // Convert domain to lower case @@ -363,6 +364,7 @@ void process_pihole_log(void) } char *client = calloc(clientlen+1,sizeof(char)); + if(client == NULL){ free(domain); continue; } // strncat() NULL-terminates the copied string (strncpy() doesn't!) strncat(client,domainend+6,clientlen); // Convert client to lower case @@ -525,6 +527,7 @@ void process_pihole_log(void) } char *forward = calloc(forwardlen+1,sizeof(char)); + if(forward == NULL) continue; // strncat() NULL-terminates the copied string (strncpy() doesn't!) strncat(forward,forwardstart+4,forwardlen); // Convert forward to lower case @@ -970,13 +973,14 @@ char *resolveHostname(const char *addr) if(he == NULL) { // No hostname found - hostname = calloc(1,sizeof(char)); - hostname[0] = '\0'; + hostname = strdup(""); + if(hostname == NULL) return NULL; } else { // Return hostname copied to new memory location hostname = strdup(he->h_name); + if(hostname == NULL) return NULL; // Convert hostname to lower case strtolower(hostname); } @@ -1184,7 +1188,7 @@ int findDomainID(const char *domain) domains[domainID].blockedcount = 0; // Initialize wildcard blocking flag with false domains[domainID].wildcard = false; - // Store domain name + // Store domain name - no need to check for NULL here as it doesn't harm domains[domainID].domain = strdup(domain); memory.domainnames += (strlen(domain) + 1) * sizeof(char); // Store DNSSEC result for this domain @@ -1269,10 +1273,10 @@ int findClientID(const char *client) clients[clientID].magic = MAGICBYTE; // Set its counter to 1 clients[clientID].count = 1; - // Store client IP + // Store client IP - no need to check for NULL here as it doesn't harm clients[clientID].ip = strdup(client); memory.clientips += (strlen(client) + 1) * sizeof(char); - // Store client hostname + // Store client hostname - no need to check for NULL here as it doesn't harm clients[clientID].name = strdup(hostname); memory.clientnames += (strlen(hostname) + 1) * sizeof(char); free(hostname); diff --git a/setupVars.c b/setupVars.c index b530ce87..f1be08d4 100644 --- a/setupVars.c +++ b/setupVars.c @@ -119,12 +119,14 @@ void getSetupVarsArray(char * input) while (p) { setupVarsArray = realloc(setupVarsArray, sizeof(char*) * ++setupVarsElements); + if(setupVarsArray == NULL) return; setupVarsArray[setupVarsElements-1] = p; p = strtok(NULL, ","); } /* realloc one extra element for the last NULL */ setupVarsArray = realloc(setupVarsArray, sizeof(char*) * (setupVarsElements+1)); + if(setupVarsArray == NULL) return; setupVarsArray[setupVarsElements] = NULL; } @@ -168,6 +170,7 @@ bool insetupVarsArray(char * str) // Copying strlen-1 chars into buffer of size strlen: OK size_t lenght = strlen(setupVarsArray[i]); char * domain = calloc(lenght, sizeof(char)); + if(domain == NULL) return false; // strncat() NULL-terminates the copied string (strncpy() doesn't!) strncat(domain, setupVarsArray[i]+1, lenght-1); diff --git a/socket.c b/socket.c index 444a6a90..f263d901 100644 --- a/socket.c +++ b/socket.c @@ -317,8 +317,8 @@ void *telnet_connection_handler_thread(void *socket_desc) { if (n > 0) { - char *message = calloc(strlen(client_message)+1,sizeof(char)); - strcpy(message, client_message); + char *message = strdup(client_message); + if(message == NULL) break; // Clear client message receive buffer memset(client_message, 0, sizeof client_message); @@ -377,8 +377,8 @@ void *socket_connection_handler_thread(void *socket_desc) { if (n > 0) { - char *message = calloc(strlen(client_message)+1,sizeof(char)); - strcpy(message, client_message); + char *message = strdup(client_message); + if(message == NULL) break; // Clear client message receive buffer memset(client_message, 0, sizeof client_message); @@ -460,6 +460,7 @@ void *telnet_listening_thread_IPv4(void *args) // Allocate memory used to transport client socket ID to client listening thread int *newsock; newsock = calloc(1,sizeof(int)); + if(newsock == NULL) break; *newsock = csck; pthread_t telnet_connection_thread; @@ -500,6 +501,7 @@ void *telnet_listening_thread_IPv6(void *args) // Allocate memory used to transport client socket ID to client listening thread int *newsock; newsock = calloc(1,sizeof(int)); + if(newsock == NULL) break; *newsock = csck; pthread_t telnet_connection_thread; @@ -536,6 +538,7 @@ void *socket_listening_thread(void *args) // Allocate memory used to transport client socket ID to client listening thread int *newsock; newsock = calloc(1,sizeof(int)); + if(newsock == NULL) break; *newsock = csck; pthread_t socket_connection_thread; From f11b6c892dce0314ca0da62796222a2d0af1c05a Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 8 Feb 2018 20:00:17 +0100 Subject: [PATCH 6/8] Don't show queries with status 5 (blocked by black.list) when API_QUERY_LOG_SHOW=permittedonly or API_QUERY_LOG_SHOW=nothing is set Signed-off-by: DL6ER --- api.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api.c b/api.c index dbc6add3..bdc5bf8d 100644 --- a/api.c +++ b/api.c @@ -667,8 +667,10 @@ void getAllQueries(char *client_message, int *sock) else strcpy(qtype,"IPv6"); - if((queries[i].status == 1 || queries[i].status == 4) && !showblocked) + // 1 = gravity.list, 4 = wildcard, 5 = black.list + if((queries[i].status == 1 || queries[i].status == 4 || queries[i].status == 5) && !showblocked) continue; + // 2 = forwarded, 3 = cached if((queries[i].status == 2 || queries[i].status == 3) && !showpermitted) continue; From abdff6e5b27b171b0d4ca88b5223a5ca461caa30 Mon Sep 17 00:00:00 2001 From: Mcat12 Date: Sun, 11 Feb 2018 22:03:11 -0500 Subject: [PATCH 7/8] Fix accidentally trying to free NULL Signed-off-by: Mcat12 --- flush.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/flush.c b/flush.c index 4fb0cee4..6ed79a81 100644 --- a/flush.c +++ b/flush.c @@ -65,7 +65,8 @@ void pihole_log_flushed(bool message) // overTime struct: Free allocated substructure for(i=0;i Date: Mon, 12 Feb 2018 12:46:33 +0100 Subject: [PATCH 8/8] Don't rely on the system's strdup() function but provide our own Signed-off-by: DL6ER --- memory.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/memory.c b/memory.c index 857d39d6..e8f1497f 100644 --- a/memory.c +++ b/memory.c @@ -225,15 +225,22 @@ char *FTLstrdup(const char *src, const char * file, const char * function, int l { // The FTLstrdup() function returns a pointer to a new string which is a // duplicate of the string s. Memory for the new string is obtained with - // malloc(3), and can be freed with free(3). + // calloc(3), and can be freed with free(3). if(src == NULL) { logg("WARN: Trying to copy a NULL string in %s() (%s:%i)", function, file, line); return NULL; } - char *dest = __strdup(src); + size_t len = strlen(src); + char *dest = calloc(len+1, sizeof(char)); if(dest == NULL) + { logg("FATAL: Memory allocation failed in %s() (%s:%i)", function, file, line); + return NULL; + } + // Use memcpy as memory areas cannot overlap + memcpy(dest, src, len); + dest[len] = '\0'; return dest; }