Improve memory handling in a few places. This is the result of a valgrind/memcheck test.

Signed-off-by: DL6ER <dl6er@dl6er.de>

build.sh

@@ -181,7 +181,6 @@ fi
 # If we are asked to run tests, we do this here
 if [[ -n "${test}" ]]; then
     cd ..
-    bash test/arch_test.sh
     bash test/run.sh
 fi
 

src/api/config.c

@@ -718,7 +718,7 @@ static int api_config_patch(struct ftl_conn *api)
 		if(new_item->f & FLAG_READ_ONLY && cJSON_IsBool(elem) && elem->valueint == 1)
 		{
 			char *key = strdup(new_item->k);
-			free_config(&newconf);
+			free_config(&newconf, false);
 			return send_json_error_free(api, 400,
 			                            "bad_request",
 			                            "This config option can only be set in pihole.toml, not via the API",
@@ -740,7 +740,7 @@ static int api_config_patch(struct ftl_conn *api)
 			char *hint = calloc(strlen(new_item->k) + strlen(response) + 3, sizeof(char));
 			if(hint == NULL)
 			{
-				free_config(&newconf);
+				free_config(&newconf, false);
 				return send_json_error(api, 500,
 				                       "internal_error",
 				                       "Failed to allocate memory for hint",
@@ -749,7 +749,7 @@ static int api_config_patch(struct ftl_conn *api)
 			strcpy(hint, new_item->k);
 			strcat(hint, ": ");
 			strcat(hint, response);
-			free_config(&newconf);
+			free_config(&newconf, false);
 			return send_json_error_free(api, 400,
 			                            "bad_request",
 			                            "Config item is invalid",
@@ -764,7 +764,7 @@ static int api_config_patch(struct ftl_conn *api)
 		if(new_item->f & FLAG_ENV_VAR && !compare_config_item(conf_item->t, &new_item->v, &conf_item->v))
 		{
 			char *key = strdup(new_item->k);
-			free_config(&newconf);
+			free_config(&newconf, false);
 			return send_json_error_free(api, 400,
 			                            "bad_request",
 			                            "Config items set via environment variables cannot be changed via the API",
@@ -789,7 +789,7 @@ static int api_config_patch(struct ftl_conn *api)
 		char errbuf[VALIDATOR_ERRBUF_LEN] = { 0 };
 		if(!conf_item->c(&new_item->v, new_item->k, errbuf))
 		{
-			free_config(&newconf);
+			free_config(&newconf, false);
 			return send_json_error(api, 400,
 			                       "bad_request",
 			                       "Config item validation failed",
@@ -832,7 +832,7 @@ static int api_config_patch(struct ftl_conn *api)
 			}
 			else
 			{
-				free_config(&newconf);
+				free_config(&newconf, false);
 				return send_json_error(api, 400,
 				                       "bad_request",
 				                       "Invalid configuration",
@@ -856,7 +856,7 @@ static int api_config_patch(struct ftl_conn *api)
 	else
 	{
 		// Nothing changed, merely release copied config memory
-		free_config(&newconf);
+		free_config(&newconf, false);
 		log_info("No config changes detected");
 	}
 
@@ -932,7 +932,7 @@ static int api_config_put_delete(struct ftl_conn *api)
 		if(new_item->f & FLAG_ENV_VAR)
 		{
 			char *key = strdup(new_item->k);
-			free_config(&newconf);
+			free_config(&newconf, false);
 			free_config_path(requested_path);
 			return send_json_error_free(api, 400,
 			                            "bad_request",
@@ -992,7 +992,7 @@ static int api_config_put_delete(struct ftl_conn *api)
 			char errbuf[VALIDATOR_ERRBUF_LEN] = { 0 };
 			if(!new_item->c(&new_item->v, new_item->k, errbuf))
 			{
-				free_config(&newconf);
+				free_config(&newconf, false);
 				free_config_path(requested_path);
 				return send_json_error(api, 400,
 				                       "bad_request",
@@ -1018,7 +1018,7 @@ static int api_config_put_delete(struct ftl_conn *api)
 	// Error 404 if config element not found
 	if(!found)
 	{
-		free_config(&newconf);
+		free_config(&newconf, false);
 		cJSON *json = JSON_NEW_OBJECT();
 		JSON_SEND_OBJECT_CODE(json, 404);
 	}
@@ -1026,7 +1026,7 @@ static int api_config_put_delete(struct ftl_conn *api)
 	// Error 400 if unique item already present
 	if(message != NULL)
 	{
-		free_config(&newconf);
+		free_config(&newconf, false);
 		return send_json_error(api, 400,
 		                       "bad_request",
 		                       message,

src/api/network.c

@@ -186,7 +186,7 @@ static int api_network_devices_GET(struct ftl_conn *api)
 	{
 		networkTable_readDevicesFinalize(device_stmt);
 		dbclose(&db);
-	
+
 		// Add SQL message (may be NULL = not available)
 		return send_json_error(api, 500,
 		                       "database_error",

src/api/padd.c

@@ -22,8 +22,6 @@
 #include "config/config.h"
 // uname()
 #include <sys/utsname.h>
-// nlroutes(), nladdrs(), nllinks()
-#include "tools/netlink.h"
 // struct proc_mem, getProcessMemory()
 #include "procps.h"
 // getcpu_percentage()

src/config/cli.c

@@ -427,7 +427,7 @@ int set_config_from_CLI(const char *key, const char *value)
 		if(item->f & FLAG_ENV_VAR)
 		{
 			log_err("Config option %s is read-only (set via environmental variable)", key);
-			free_config(&newconf);
+			free_config(&newconf, false);
 			return ENV_VAR_FORCED;
 		}
 
@@ -435,7 +435,7 @@ int set_config_from_CLI(const char *key, const char *value)
 		if(item->f & FLAG_READ_ONLY)
 		{
 			log_err("Config option %s can only be set in pihole.toml, not via the CLI", key);
-			free_config(&newconf);
+			free_config(&newconf, false);
 			return EXIT_FAILURE;
 		}
 
@@ -459,14 +459,14 @@ int set_config_from_CLI(const char *key, const char *value)
 			log_err(" - %s", matches[i]);
 		free(matches);
 
-		free_config(&newconf);
+		free_config(&newconf, false);
 		return KEY_UNKNOWN;
 	}
 
 	// Parse value
 	if(!readStringValue(new_item, value, &newconf))
 	{
-		free_config(&newconf);
+		free_config(&newconf, false);
 		return VALUE_INVALID;
 	}
 
@@ -484,7 +484,7 @@ int set_config_from_CLI(const char *key, const char *value)
 			char errbuf[VALIDATOR_ERRBUF_LEN] = { 0 };
 			if(!new_item->c(&new_item->v, new_item->k, errbuf))
 			{
-				free_config(&newconf);
+				free_config(&newconf, false);
 				log_err("Invalid value: %s", errbuf);
 				return 3;
 			}
@@ -498,7 +498,7 @@ int set_config_from_CLI(const char *key, const char *value)
 			{
 				// Test failed
 				log_debug(DEBUG_CONFIG, "Config item %s: dnsmasq config test failed", conf_item->k);
-				free_config(&newconf);
+				free_config(&newconf, false);
 				return DNSMASQ_TEST_FAILED;
 			}
 		}
@@ -519,7 +519,7 @@ int set_config_from_CLI(const char *key, const char *value)
 	{
 		// No change
 		log_debug(DEBUG_CONFIG, "Config item %s: Unchanged", conf_item->k);
-		free_config(&newconf);
+		free_config(&newconf, false);
 
 		// Print value
 		writeTOMLvalue(stdout, -1, conf_item->t, &conf_item->v);

src/config/config.c

@@ -334,14 +334,14 @@ bool compare_config_item(const enum conf_type t, const union conf_value *val1, c
 }
 
 
-void free_config(struct config *conf)
+void free_config(struct config *conf, const bool terminating)
 {
 	// Post-processing:
 	// Initialize and verify config data
 	for(unsigned int i = 0; i < CONFIG_ELEMENTS; i++)
 	{
-		// Get pointer to memory location of this conf_item (copy)
+		// Get pointer to memory location of this conf_item
-		struct conf_item *copy_item = get_conf_item(conf, i);
+		struct conf_item *conf_item = get_conf_item(conf, i);
 
 		// Free allowed values (if defined)
 		// Note: This is no necessary as we simply leave the allowed values
@@ -350,7 +350,7 @@ void free_config(struct config *conf)
 		// if(conf->a != NULL) cJSON_Delete(conf->a);
 
 		// Make a type-dependent copy of the value
-		switch(copy_item->t)
+		switch(conf_item->t)
 		{
 			case CONF_BOOL:
 			case CONF_INT:
@@ -375,12 +375,19 @@ void free_config(struct config *conf)
 				// Nothing to do
 				break;
 			case CONF_STRING_ALLOCATED:
-				free(copy_item->v.s);
+				free(conf_item->v.s);
-				copy_item->v.s = NULL;
+				conf_item->v.s = NULL;
-				copy_item->t = CONF_STRING; // not allocated anymore
+				conf_item->t = CONF_STRING; // not allocated anymore
 				break;
 			case CONF_JSON_STRING_ARRAY:
-				cJSON_Delete(copy_item->v.json);
+				// Delete default JSON only when terminating.
+				// During config replacements, it is simply
+				// handed over from the old to the new config
+				// structure to avoid unnecessary memory
+				// duplications
+				if(terminating)
+					cJSON_Delete(conf_item->d.json);
+				cJSON_Delete(conf_item->v.json);
 				break;
 		}
 	}
@@ -1043,12 +1050,12 @@ void initConfig(struct config *conf)
 	conf->webserver.headers.t = CONF_JSON_STRING_ARRAY;
 	conf->webserver.headers.f = FLAG_RESTART_FTL;
 	conf->webserver.headers.d.json = cJSON_CreateArray();
-	cJSON_AddItemReferenceToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("X-DNS-Prefetch-Control: off"));
+	cJSON_AddItemToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("X-DNS-Prefetch-Control: off"));
-	cJSON_AddItemReferenceToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("Content-Security-Policy: default-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:;"));
+	cJSON_AddItemToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("Content-Security-Policy: default-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:;"));
-	cJSON_AddItemReferenceToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("X-Frame-Options: DENY"));
+	cJSON_AddItemToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("X-Frame-Options: DENY"));
-	cJSON_AddItemReferenceToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("X-XSS-Protection: 0"));
+	cJSON_AddItemToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("X-XSS-Protection: 0"));
-	cJSON_AddItemReferenceToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("X-Content-Type-Options: nosniff"));
+	cJSON_AddItemToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("X-Content-Type-Options: nosniff"));
-	cJSON_AddItemReferenceToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("Referrer-Policy: strict-origin-when-cross-origin"));
+	cJSON_AddItemToArray(conf->webserver.headers.d.json, cJSON_CreateStringReference("Referrer-Policy: strict-origin-when-cross-origin"));
 	conf->webserver.headers.c = validate_stub; // Only type-based checking
 
 	conf->webserver.serve_all.k = "webserver.serve_all";
@@ -1067,7 +1074,7 @@ void initConfig(struct config *conf)
 
 	conf->webserver.session.restore.k = "webserver.session.restore";
 	conf->webserver.session.restore.h = "Should Pi-hole backup and restore sessions from the database? This is useful if you want to keep your sessions after a restart of the web interface.";
-	conf->webserver.session.restore.a = cJSON_CreateStringReference("true or false");	
+	conf->webserver.session.restore.a = cJSON_CreateStringReference("true or false");
 	conf->webserver.session.restore.t = CONF_BOOL;
 	conf->webserver.session.restore.d.b = true;
 	conf->webserver.session.restore.c = validate_stub; // Only type-based checking
@@ -1665,7 +1672,6 @@ void initConfig(struct config *conf)
 		{
 			conf_item->a = cJSON_CreateStringReference("true or false");
 		}
-	
 	}
 }
 
@@ -1956,7 +1962,7 @@ void replace_config(struct config *newconf)
 	memcpy(&config, newconf, sizeof(struct config));
 
 	// Free old backup struct
-	free_config(&old_conf);
+	free_config(&old_conf, false);
 
 	// Unlock shared memory
 	unlock_shm();
@@ -2016,7 +2022,7 @@ void reread_config(void)
 	{
 		// New configuration is invalid, restore old one
 		log_debug(DEBUG_CONFIG, "Modified config file is invalid, discarding and overwriting with current configuration");
-		free_config(&conf_copy);
+		free_config(&conf_copy, false);
 	}
 
 	// Write the config file back to disk to ensure that all options and

src/config/config.h

@@ -371,7 +371,7 @@ struct conf_item *get_conf_item(struct config *conf, const unsigned int n);
 struct conf_item *get_debug_item(struct config *conf, const enum debug_flag debug);
 unsigned int config_path_depth(char **paths) __attribute__ ((pure));
 void duplicate_config(struct config *dst, struct config *src);
-void free_config(struct config *conf);
+void free_config(struct config *conf, const bool terminating);
 bool compare_config_item(const enum conf_type t, const union conf_value *val1, const union conf_value *val2);
 char **gen_config_path(const char *pathin, const char delim);
 void free_config_path(char **paths);

src/daemon.c

@@ -65,6 +65,9 @@ void go_daemon(void)
 	if (process_id > 0)
 	{
 		printf("FTL started!\n");
+		// Free config to silence meaningless (but still loud) memcheck
+		// warnings about lost memory concerning the parsed config
+		free_config(&config, true);
 		// Return success in exit status
 		exit(EXIT_SUCCESS);
 	}
@@ -423,18 +426,21 @@ void cleanup(const int ret)
 	// Close memory database
 	close_memory_database();
 
-	// Remove shared memory objects
-	// Important: This invalidated all objects such as
-	//            counters-> ... etc.
-	// This should be the last action when c
-	destroy_shmem();
-
 	// De-initialize the random number generator and entropy collector
 	destroy_entropy();
 
 	// Free environment variables
 	freeEnvVars();
 
+	// Free config
+	free_config(&config, true);
+
+	// Remove shared memory objects
+	// Important: This invalidated all objects such as
+	//            counters-> ... etc.
+	// This should be the last action when cleaning up
+	destroy_shmem();
+
 	char buffer[42] = { 0 };
 	format_time(buffer, 0, timer_elapsed_msec(EXIT_TIMER));
 	if(ret == RESTART_FTL_CODE)

src/database/message-table.c

@@ -359,9 +359,9 @@ static int _add_message(const enum message_type type,
 		return -1;
 	}
 
-	sqlite3 *db;
+	sqlite3 *db = dbopen(false, false);
 	// Open database connection
-	if((db = dbopen(false, false)) == NULL)
+	if(db == NULL)
 		// Reason for failure is logged in dbopen()
 		return -1;
 
@@ -380,8 +380,6 @@ static int _add_message(const enum message_type type,
 	{
 		log_err("add_message(type=%u, message=%s) - Failed to bind type DELETE: %s",
 			type, message, sqlite3_errstr(rc));
-		sqlite3_reset(stmt);
-		sqlite3_finalize(stmt);
 		goto end_of_add_message;
 	}
 
@@ -390,8 +388,6 @@ static int _add_message(const enum message_type type,
 	{
 		log_err("add_message(type=%u, message=%s) - Failed to bind message DELETE: %s",
 			type, message, sqlite3_errstr(rc));
-		sqlite3_reset(stmt);
-		sqlite3_finalize(stmt);
 		goto end_of_add_message;
 	}
 
@@ -423,8 +419,6 @@ static int _add_message(const enum message_type type,
 	{
 		log_err("add_message(type=%u, message=%s) - Failed to bind type: %s",
 		        type, message, sqlite3_errstr(rc));
-		sqlite3_reset(stmt);
-		sqlite3_finalize(stmt);
 		goto end_of_add_message;
 	}
 
@@ -433,8 +427,6 @@ static int _add_message(const enum message_type type,
 	{
 		log_err("add_message(type=%u, message=%s) - Failed to bind message: %s",
 		        type, message, sqlite3_errstr(rc));
-		sqlite3_reset(stmt);
-		sqlite3_finalize(stmt);
 		goto end_of_add_message;
 	}
 
@@ -489,15 +481,19 @@ static int _add_message(const enum message_type type,
 		goto end_of_add_message;
 	}
 
-	// Final database handling
-	sqlite3_clear_bindings(stmt);
-	sqlite3_reset(stmt);
-	sqlite3_finalize(stmt);
-
-	// Get row ID of the newly added message
-	rowid = sqlite3_last_insert_rowid(db);
-
 end_of_add_message: // Close database connection
+
+	// Final database handling
+	if(stmt != NULL)
+	{
+		sqlite3_clear_bindings(stmt);
+		sqlite3_reset(stmt);
+		sqlite3_finalize(stmt);
+
+		// Get row ID of the newly added message
+		rowid = sqlite3_last_insert_rowid(db);
+	}
+
 	dbclose(&db);
 
 	return rowid;

src/database/network-table.c

@@ -1069,6 +1069,7 @@ static bool add_local_interfaces_to_network_table(sqlite3 *db, time_t now, unsig
 	if(!nllinks(links, false))
 	{
 		log_err("Failed to get links, cannot update network table");
+		cJSON_Delete(links);
 		return false;
 	}
 	log_debug(DEBUG_ARP, "Network table: Successfully read links with %i entries",
@@ -1166,6 +1167,9 @@ static bool add_local_interfaces_to_network_table(sqlite3 *db, time_t now, unsig
 		}
 	}
 
+	// Free allocated memory
+	cJSON_Delete(link);
+
 	return true;
 }
 
@@ -1544,6 +1548,9 @@ void parse_neighbor_cache(sqlite3 *db)
 			entries++;
 		}
 
+		// Free allocated JSON array
+		cJSON_Delete(json);
+
 		log_debug(DEBUG_ARP, "Network table: Finished parsing ARP cache with %u entries", entries);
 
 		if(rc != SQLITE_OK)

src/ntp/client.c

@@ -478,7 +478,7 @@ bool ntp_client(const char *server, const bool settime, const bool print)
 {
 	// Resolve server address
 	int eai;
-	struct addrinfo *saddr;
+	struct addrinfo *saddr = NULL;
 	// Resolve server address, port 123 is used for NTP
 	if((eai = getaddrinfo(server, "123", NULL, &saddr)) != 0)
 	{
@@ -494,6 +494,7 @@ bool ntp_client(const char *server, const bool settime, const bool print)
 		}
 		errbuf[sizeof(errbuf) - 1] = '\0';
 		log_ntp_message(true, false, errbuf);
+		freeaddrinfo(saddr);
 		return false;
 	}
 
@@ -502,6 +503,7 @@ bool ntp_client(const char *server, const bool settime, const bool print)
 	if(ntp == NULL)
 	{
 		log_err("Cannot allocate memory for NTP client");
+		freeaddrinfo(saddr);
 		return false;
 	}
 
@@ -542,6 +544,7 @@ bool ntp_client(const char *server, const bool settime, const bool print)
 
 	// Free allocated memory
 	freeaddrinfo(saddr);
+	saddr = NULL;
 
 	// Compute average and standard deviation
 	unsigned int valid = 0;

src/webserver/webserver.c

@@ -38,6 +38,7 @@ static char *prefix_webhome = NULL;
 static char *api_uri = NULL;
 static char *admin_api_uri = NULL;
 static char *login_uri = NULL;
+static char *webheaders = NULL;
 
 // Private prototypes
 static char *append_to_path(char *path, const char *append);
@@ -577,7 +578,7 @@ void http_init(void)
 	}
 
 	// Construct additional headers
-	char *webheaders = strdup("");
+	webheaders = strdup("");
 	if (webheaders == NULL) {
 		log_err("Failed to allocate memory for webheaders!");
 		return;
@@ -595,11 +596,14 @@ void http_init(void)
 		const char *h = cJSON_GetStringValue(header);
 
 		// Allocate memory for the new header
-		webheaders = realloc(webheaders, strlen(webheaders) + strlen(h) + 3);
+		char *new_webheaders = realloc(webheaders, strlen(webheaders) + strlen(h) + 3);
-		if (webheaders == NULL) {
+		if (new_webheaders == NULL) {
-			log_err("Failed to allocate memory for webheaders!");
+			log_err("Failed to (re)allocate memory for webheaders!");
+			free(webheaders);
+			webheaders = NULL;
 			return;
 		}
+		webheaders = new_webheaders;
 		strcat(webheaders, h);
 		strcat(webheaders, "\r\n");
 	}
@@ -866,8 +870,11 @@ void http_terminate(void)
 	// Free admin_api_uri path
 	if(admin_api_uri != NULL)
 		free(admin_api_uri);
-	
+
 	// Free login_uri path
 	if(login_uri != NULL)
 		free(login_uri);
+
+	if(webheaders != NULL)
+		free(webheaders);
 }