Merge pull request #2059 from pi-hole/fix/yubi_bug_1

Fix possible crash due to incorrect setupVars.conf import

src/FTL.h

@@ -152,7 +152,7 @@
 // caused by insufficient memory or by code bugs (not properly dealing
 // with NULL pointers) much easier.
 #undef strdup // strdup() is a macro in itself, it needs special handling
-#define free(ptr) FTLfree((void**)&ptr, __FILE__,  __FUNCTION__,  __LINE__)
+#define free(ptr) { FTLfree(ptr, __FILE__,  __FUNCTION__,  __LINE__); ptr = NULL; }
 #define strdup(str_in) FTLstrdup(str_in, __FILE__,  __FUNCTION__,  __LINE__)
 #define calloc(numer_of_elements, element_size) FTLcalloc(numer_of_elements, element_size, __FILE__,  __FUNCTION__,  __LINE__)
 #define realloc(ptr, new_size) FTLrealloc(ptr, new_size, __FILE__,  __FUNCTION__,  __LINE__)

src/config/password.c

@@ -440,7 +440,6 @@ enum password_result verify_password(const char *password, const char *pwhash, c
 				config.webserver.api.pwhash.v.s = new_hash;
 				config.webserver.api.pwhash.t = CONF_STRING_ALLOCATED;
 				writeFTLtoml(true);
-				free(new_hash);
 			}
 
 			// Successful logins do not count against rate-limiting

src/config/setupVars.c

@@ -122,7 +122,6 @@ static void get_conf_bool_from_setupVars(const char *key, struct conf_item *conf
 
 static void get_revServer_from_setupVars(void)
 {
-	char *active = NULL;
 	char *cidr = NULL;
 	char *target = NULL;
 	char *domain = NULL;
@@ -144,7 +143,7 @@ static void get_revServer_from_setupVars(void)
 		clearSetupVarsArray();
 		return;
 	}
-	active = strdup(active_str);
+	bool active = strcasecmp(active_str, "true") == 0;
 
 	// Free memory, harmless to call if read_setupVarsconf() didn't return a result
 	clearSetupVarsArray();
@@ -186,16 +185,16 @@ static void get_revServer_from_setupVars(void)
 	clearSetupVarsArray();
 
 	// Only add the entry if all values are present and active
-	if(active != NULL && cidr != NULL && target != NULL && domain != NULL)
+	if(cidr != NULL && target != NULL && domain != NULL)
 	{
 		// Build comma-separated string of all values
 		// 9 = 3 commas, "true/false", and null terminator
 		char *old = calloc(strlen(cidr) + strlen(target) + strlen(domain) + 9, sizeof(char));
-		if(old)
+		if(old != NULL)
 		{
 			// Add to new config
 			// active is always true as we only add active entries
-			sprintf(old, "%s,%s,%s,%s", active_str, cidr, target, domain);
+			sprintf(old, "%s,%s,%s,%s", active ? "true" : "false", cidr, target, domain);
 			cJSON_AddItemToArray(config.dns.revServers.v.json, cJSON_CreateString(old));
 
 			// Parameter present in setupVars.conf
@@ -211,8 +210,6 @@ static void get_revServer_from_setupVars(void)
 	}
 
 	// Free memory
-	if(active != NULL)
-		free(active);
 	if(cidr != NULL)
 		free(cidr);
 	if(target != NULL)

src/config/toml_helper.c

@@ -543,7 +543,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 		}
 		case CONF_ENUM_PTR_TYPE:
 		{
-			const toml_datum_t val = toml_string_in(toml, key);
+			toml_datum_t val = toml_string_in(toml, key);
 			if(val.ok)
 			{
 				const int ptr_type = get_ptr_type_val(val.u.s);
@@ -559,7 +559,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 		}
 		case CONF_ENUM_BUSY_TYPE:
 		{
-			const toml_datum_t val = toml_string_in(toml, key);
+			toml_datum_t val = toml_string_in(toml, key);
 			if(val.ok)
 			{
 				const int busy_reply = get_busy_reply_val(val.u.s);
@@ -575,7 +575,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 		}
 		case CONF_ENUM_BLOCKING_MODE:
 		{
-			const toml_datum_t val = toml_string_in(toml, key);
+			toml_datum_t val = toml_string_in(toml, key);
 			if(val.ok)
 			{
 				const int blocking_mode = get_blocking_mode_val(val.u.s);
@@ -591,7 +591,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 		}
 		case CONF_ENUM_REFRESH_HOSTNAMES:
 		{
-			const toml_datum_t val = toml_string_in(toml, key);
+			toml_datum_t val = toml_string_in(toml, key);
 			if(val.ok)
 			{
 				const int refresh_hostnames = get_refresh_hostnames_val(val.u.s);
@@ -607,7 +607,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 		}
 		case CONF_ENUM_LISTENING_MODE:
 		{
-			const toml_datum_t val = toml_string_in(toml, key);
+			toml_datum_t val = toml_string_in(toml, key);
 			if(val.ok)
 			{
 				const int listeningMode = get_listeningMode_val(val.u.s);
@@ -623,7 +623,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 		}
 		case CONF_ENUM_WEB_THEME:
 		{
-			const toml_datum_t val = toml_string_in(toml, key);
+			toml_datum_t val = toml_string_in(toml, key);
 			if(val.ok)
 			{
 				const int web_theme = get_web_theme_val(val.u.s);
@@ -639,7 +639,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 		}
 		case CONF_ENUM_TEMP_UNIT:
 		{
-			const toml_datum_t val = toml_string_in(toml, key);
+			toml_datum_t val = toml_string_in(toml, key);
 			if(val.ok)
 			{
 				const int temp_unit = get_temp_unit_val(val.u.s);
@@ -665,7 +665,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 		case CONF_STRUCT_IN_ADDR:
 		{
 			struct in_addr addr4 = { 0 };
-			const toml_datum_t val = toml_string_in(toml, key);
+			toml_datum_t val = toml_string_in(toml, key);
 			if(val.ok)
 			{
 				if(strlen(val.u.s) == 0)
@@ -686,7 +686,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 		case CONF_STRUCT_IN6_ADDR:
 		{
 			struct in6_addr addr6 = { 0 };
-			const toml_datum_t val = toml_string_in(toml, key);
+			toml_datum_t val = toml_string_in(toml, key);
 			if(val.ok)
 			{
 				if(strlen(val.u.s) == 0)
@@ -717,7 +717,7 @@ void readTOMLvalue(struct conf_item *conf_item, const char* key, toml_table_t *t
 				for(unsigned int i = 0; i < nelem; i++)
 				{
 					// Get string from TOML
-					const toml_datum_t d = toml_string_at(array, i);
+					toml_datum_t d = toml_string_at(array, i);
 					if(!d.ok)
 					{
 						log_warn("Config %s is an invalid array (found at index %u)", conf_item->k, i);

src/signals.c

@@ -29,7 +29,7 @@
 #define BINARY_NAME "pihole-FTL"
 
 volatile sig_atomic_t killed = 0;
-static volatile pid_t mpid = -1;
+static volatile pid_t mpid = 0;
 static time_t FTLstarttime = 0;
 volatile int exit_code = EXIT_SUCCESS;
 
@@ -253,7 +253,7 @@ static void __attribute__((noreturn)) signal_handler(int sig, siginfo_t *si, voi
 	log_info("Thank you for helping us to improve our FTL engine!");
 
 	// Terminate main process if crash happened in a TCP worker
-	if(mpid != getpid())
+	if(main_pid() != getpid())
 	{
 		// This is a forked process
 		log_info("Asking parent pihole-FTL (PID %i) to shut down", (int)mpid);
@@ -474,7 +474,7 @@ void handle_realtime_signals(void)
 // Return PID of the main FTL process
 pid_t main_pid(void)
 {
-	if(mpid > -1)
+	if(mpid > 0)
 		// Has already been set
 		return mpid;
 	else

src/syscalls/free.c

@@ -13,26 +13,20 @@
 #include "log.h"
 
 #undef free
-void FTLfree(void **ptr, const char *file, const char *func, const int line)
+bool FTLfree(void *ptr, const char *file, const char *func, const 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)
-	{
-		log_warn("Trying to free NULL memory location in %s() (%s:%i)", func, file, line);
-		return;
-	}
-	if(*ptr == NULL)
 	{
 		log_warn("Trying to free NULL pointer in %s() (%s:%i)", func, file, line);
-		return;
+		return false;
 	}
 
 	// Actually free the memory
-	free(*ptr);
+	free(ptr);
 
-	// Set the pointer to NULL
-	*ptr = NULL;
+	return true;
 }

src/syscalls/syscalls.h

@@ -14,7 +14,7 @@
 char *FTLstrdup(const char *src, const char *file, const char *func, const int line) __attribute__((malloc));
 void *FTLcalloc(size_t n, size_t size, const char *file, const char *func, const int line) __attribute__((malloc)) __attribute__((alloc_size(1,2)));
 void *FTLrealloc(void *ptr_in, size_t size, const char *file, const char *func, const int line) __attribute__((alloc_size(2)));
-void FTLfree(void **ptr, const char*file, const char *func, const int line);
+bool FTLfree(void *ptr, const char*file, const char *func, const int line);
 int FTLfallocate(const int fd, const off_t offset, const off_t len, const char *file, const char *func, const int line);
 
 

src/webserver/webserver.c

@@ -303,6 +303,7 @@ unsigned short get_api_string(char **buf, const bool domain)
 		if(this_len < 0)
 		{
 			log_err("Failed to append API URL to buffer: %s", strerror(errno));
+			free(api_str);
 			return 0;
 		}
 
@@ -311,15 +312,16 @@ unsigned short get_api_string(char **buf, const bool domain)
 		if((size_t)this_len >= bufsz - len - 1)
 		{
 			log_err("API URL buffer too small!");
+			free(api_str);
 			return 0;
 		}
 
 		// Check if this string is already present in the buffer
 		if(memmem(*buf, len, api_str, this_len) != NULL)
 		{
 			// This string is already present, so skip it
-			free(api_str);
 			log_debug(DEBUG_API, "Skipping duplicate API URL: %s", api_str);
+			free(api_str);
 			continue;
 		}