Fix a memory leak when re-opening the databases (when forking or reloading the lists). The memory leak is on the order of a few bytes but scales quickly with the number of clients. It is caused by SQLite3 not being able to clean up behind itself when we're not finalizing and closing everything explicitly.

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

src/database/common.c

@@ -48,6 +48,8 @@ void dbclose(void)
 	int rc = SQLITE_OK;
 	if( FTL_db != NULL )
 	{
+		if(config.debug & DEBUG_DATABASE)
+			logg("Closing FTL database");
 		if((rc = sqlite3_close(FTL_db)) != SQLITE_OK)
 			logg("Encountered error while trying to close database: %s", sqlite3_errstr(rc));
 
@@ -83,6 +85,8 @@ bool dbopen(void)
 		logg("Locking FTL database: Success");
 
 	// Try to open database
+	if(config.debug & DEBUG_DATABASE)
+		logg("Opening FTL database");
 	int rc = sqlite3_open_v2(FTLfiles.FTL_db, &FTL_db, SQLITE_OPEN_READWRITE, NULL);
 	if( rc != SQLITE_OK )
 	{
@@ -107,10 +111,13 @@ bool dbopen(void)
 }
 
 // (Re-)Open pihole-FTL database connection
-void piholeFTLDB_reopen(void)
+bool piholeFTLDB_reopen(void)
 {
+	// We call this routine when reloading the cache and when forking. Even
+	// when the database handle isn't really valid in this fork, we still
+	// want to close it here to avoid leaking memory
 	dbclose();
-	dbopen();
+	return dbopen();
 }
 
 int dbquery(const char *format, ...)

src/database/common.h

@@ -35,7 +35,7 @@ int dbquery(const char *format, ...);
 bool FTL_DB_avail(void) __attribute__ ((pure));
 bool dbopen(void);
 void dbclose(void);
-void piholeFTLDB_reopen(void);
+bool piholeFTLDB_reopen(void);
 int db_query_int(const char*);
 long get_lastID(void);
 void SQLite3LogCallback(void *pArg, int iErrCode, const char *zMsg);

src/database/gravity-db.c

@@ -55,24 +55,8 @@ static const char* tablename[] = { "vw_gravity", "vw_blacklist", "vw_whitelist",
 // Prototypes from functions in dnsmasq's source
 void rehash(int size);
 
-// Initialize gravity subroutines
-void gravityDB_forked(void)
-{
-	// Pretend that we did not open the database so far so it needs to be
-	// re-opened, also pretend we have not yet prepared the list statements
-	gravityDB_opened = false;
-	gravity_db = NULL;
-	whitelist_stmt = NULL;
-	blacklist_stmt = NULL;
-	gravity_stmt = NULL;
-	gravityDB_open();
-}
-
-void gravityDB_reopen(void)
-{
-	gravityDB_close();
-	gravityDB_open();
-}
+// Other private prototypes
+static inline void gravityDB_finalize_client_statements(clientsData *client);
 
 // Open gravity database
 bool gravityDB_open(void)
@@ -179,6 +163,30 @@ bool gravityDB_open(void)
 	return true;
 }
 
+bool gravityDB_reopen(void)
+{
+	// We call this routine when reloading the cache and when forking. Even
+	// when the database handle isn't really valid in this fork, we still
+	// want to close it here to avoid leaking memory
+	gravityDB_close();
+
+	// Finalize prepared list statements for all clients
+	// We can do this here as the fork has its own copy-on-write
+	// variables we still need to finalize/free in order to avoid
+	// leaking memory
+	for(int clientID = 0; clientID < counters->clients; clientID++)
+	{
+		clientsData *client = getClient(clientID, true);
+		gravityDB_finalize_client_statements(client);
+	}
+	free_sqlite3_stmt_vec(&whitelist_stmt);
+	free_sqlite3_stmt_vec(&blacklist_stmt);
+	free_sqlite3_stmt_vec(&gravity_stmt);
+
+	// Re-open gravity database
+	return gravityDB_open();
+}
+
 static char* get_client_querystr(const char* table, const char* groups)
 {
 	// Build query string with group filtering
@@ -907,12 +915,9 @@ void gravityDB_close(void)
 	}
 
 	// Free allocated memory for vectors of prepared client statements
-	free_sqlite3_stmt_vec(whitelist_stmt);
-	whitelist_stmt = NULL;
-	free_sqlite3_stmt_vec(blacklist_stmt);
-	blacklist_stmt = NULL;
-	free_sqlite3_stmt_vec(gravity_stmt);
-	gravity_stmt = NULL;
+	free_sqlite3_stmt_vec(&whitelist_stmt);
+	free_sqlite3_stmt_vec(&blacklist_stmt);
+	free_sqlite3_stmt_vec(&gravity_stmt);
 
 	// Finalize audit list statement
 	sqlite3_finalize(auditlist_stmt);

src/database/gravity-db.h

@@ -18,9 +18,8 @@
 // Table indices
 enum gravity_tables { GRAVITY_TABLE, EXACT_BLACKLIST_TABLE, EXACT_WHITELIST_TABLE, REGEX_BLACKLIST_TABLE, REGEX_WHITELIST_TABLE, UNKNOWN_TABLE } __attribute__ ((packed));
 
-void gravityDB_forked(void);
-void gravityDB_reopen(void);
 bool gravityDB_open(void);
+bool gravityDB_reopen(void);
 void gravityDB_reload_groups(clientsData* client);
 bool gravityDB_prepare_client_statements(clientsData* client);
 void gravityDB_close(void);

src/dnsmasq_interface.c

@@ -2079,8 +2079,9 @@ void FTL_TCP_worker_terminating(bool finished)
 		return;
 	}
 
-	// Close dedicated database connection of this fork
+	// Close dedicated database connections of this fork
 	gravityDB_close();
+	dbclose();
 }
 
 // Called when a (forked) TCP worker is created
@@ -2145,11 +2146,24 @@ void FTL_TCP_worker_created(const int confd, const char *iface_name)
 
 	// Reopen gravity database handle in this fork as the main process's
 	// handle isn't valid here
-	gravityDB_forked();
+	if(config.debug != 0)
+		logg("Reopening Gravity database for this fork");
+	lock_shm();
+	gravityDB_reopen();
+	unlock_shm();
+
+	// Reopen FTL's database handle in this fork
+	if(config.debug != 0)
+		logg("Reopening FTL database for this fork");
+	piholeFTLDB_reopen();
 
 	// Children inherit file descriptors from their parents
 	// We don't need them in the forks, so we clean them up
+	if(config.debug != 0)
+		logg("Closing Telnet socket for this fork");
 	close_telnet_socket();
+	if(config.debug != 0)
+		logg("Closing Unix socket for this fork");
 	close_unix_socket(false);
 }
 
@@ -2191,7 +2205,6 @@ bool FTL_unlink_DHCP_lease(const char *ipaddr)
 
 	// Return success
 	return true;
-	
 }
 
 void FTL_query_in_progress(const int id)

src/vector.c

@@ -26,7 +26,6 @@ sqlite3_stmt_vec *new_sqlite3_stmt_vec(unsigned int initial_size)
 	// Set correct subroutine pointers
 	v->set = set_sqlite3_stmt_vec;
 	v->get = get_sqlite3_stmt_vec;
-	v->free = free_sqlite3_stmt_vec;
 	return v;
 }
 
@@ -106,18 +105,19 @@ sqlite3_stmt * __attribute__((pure)) get_sqlite3_stmt_vec(sqlite3_stmt_vec *v, u
 	return item;
 }
 
-void free_sqlite3_stmt_vec(sqlite3_stmt_vec *v)
+void free_sqlite3_stmt_vec(sqlite3_stmt_vec **v)
 {
 	if(config.debug & DEBUG_VECTORS)
-		logg("Freeing sqlite3_stmt* vector %p", v);
+		logg("Freeing sqlite3_stmt* vector %p", *v);
 
 	// This vector was never allocated, invoking free_sqlite3_stmt_vec() on a
 	// NULL pointer should be a harmless no-op.
-	if(v == NULL || v->items == NULL)
+	if(v == NULL || *v == NULL || (*v)->items == NULL)
 		return;
 
 	// Free elements of the vector...
-	free(v->items);
+	free((*v)->items);
 	// ...and then the vector itself
-	free(v);
+	free(*v);
+	*v = NULL;
 }

src/vector.h

@@ -28,14 +28,13 @@ typedef struct sqlite3_stmt_vec {
 	sqlite3_stmt **items;
 	sqlite3_stmt *(*get)(struct sqlite3_stmt_vec *, unsigned int);
 	void (*set)(struct sqlite3_stmt_vec *, unsigned int, sqlite3_stmt*);
-	void (*free)(struct sqlite3_stmt_vec *);
 } sqlite3_stmt_vec;
-ASSERT_SIZEOF(sqlite3_stmt_vec, 40, 20, 20);
+ASSERT_SIZEOF(sqlite3_stmt_vec, 32, 16, 16);
 
 sqlite3_stmt_vec *new_sqlite3_stmt_vec(unsigned int initial_size);
 void set_sqlite3_stmt_vec(sqlite3_stmt_vec *v, unsigned int index, sqlite3_stmt* item);
 sqlite3_stmt* get_sqlite3_stmt_vec(sqlite3_stmt_vec *v, unsigned int index) __attribute__((pure));
 void del_sqlite3_stmt_vec(sqlite3_stmt_vec *v, unsigned int index);
-void free_sqlite3_stmt_vec(sqlite3_stmt_vec *v);
+void free_sqlite3_stmt_vec(sqlite3_stmt_vec **v);
 
 #endif //VECTOR_H