Re: clamd: Keep scanning while reloading database

Hi,

> 1) Start reload_db() in a background thread, resume scanning, and call
>    back once the new engine is in place; then exchange the pointers
>    from old to new engine and free the old one.

FWIW, I have implemented this option, and it seems to work just fine.
Patch is pasted below.

Do you see any problems with this approach?

Julius

-- 
From 56e376e55db86571e50737cf20a33b8716cad291 Mon Sep 17 00:00:00 2001
From: Julius Plenz <[email protected]>
Date: Mon, 7 Apr 2014 19:21:44 +0200
Subject: [PATCH] Reload virus definition in separate thread, eliminating delay

This implements option #1 of
http://lurker.clamav.net/message/20140407.145009.12f41a68.en.html

We introduce a daemon-global "reload_engine" pointer, also protected
under the reload_mutex. Instead of calling reload_db() syncronously, we
dispatch a thread that will eventually write to the reload_engine
pointer, signaling that the new engine is fully set up. Then, under
protection of the mutex, the current engine is exchanged and the old
engine freed.
---
 clamd/server-th.c | 120 +++++++++++++++++++++++++++++-------------------------
 clamd/server.h    |   7 ++++
 2 files changed, 71 insertions(+), 56 deletions(-)

diff --git a/clamd/server-th.c b/clamd/server-th.c
index 86a3a48..9afa2b7 100644
--- a/clamd/server-th.c
+++ b/clamd/server-th.c
@@ -64,6 +64,7 @@
 int progexit = 0;
 pthread_mutex_t exit_mutex = PTHREAD_MUTEX_INITIALIZER;
 int reload = 0;
+struct cl_engine *reload_engine = NULL;
 time_t reloaded_time = 0;
 pthread_mutex_t reload_mutex = PTHREAD_MUTEX_INITIALIZER;
 int sighup = 0;
@@ -158,40 +159,27 @@ void sighandler_th(int sig)
 	    logg("$Failed to write to syncpipe\n");
 }
 
-static struct cl_engine *reload_db(struct cl_engine *engine, unsigned int dboptions, const struct optstruct *opts, int do_check, int *ret)
+static void *reload_db(void *argp)
 {
 	const char *dbdir;
 	int retval;
 	unsigned int sigs = 0;
 	struct cl_settings *settings = NULL;
+	struct cl_engine *engine = NULL;
 
-    *ret = 0;
-    if(do_check) {
-	if(!dbstat.entries) {
-	    logg("No stats for Database check - forcing reload\n");
-	    return engine;
-	}
-
-	if(cl_statchkdir(&dbstat) == 1) {
-	    logg("SelfCheck: Database modification detected. Forcing reload.\n");
-	    return engine;
-	} else {
-	    logg("SelfCheck: Database status OK.\n");
-	    return NULL;
-	}
-    }
+	struct reload_db_args *args = (struct reload_db_args *)argp;
+	struct cl_engine *oldengine = args->engine;
+	unsigned int dboptions = args->dboptions;
+	const struct optstruct *opts = args->opts;
 
-    /* release old structure */
-    if(engine) {
+    if(oldengine) {
 	/* copy current settings */
-	settings = cl_engine_settings_copy(engine);
+	settings = cl_engine_settings_copy(oldengine);
 	if(!settings)
 	    logg("^Can't make a copy of the current engine settings\n");
-
-	thrmgr_setactiveengine(NULL);
-	cl_engine_free(engine);
     }
 
+
     dbdir = optget(opts, "DatabaseDirectory")->strarg;
     logg("Reading databases from %s\n", dbdir);
 
@@ -201,18 +189,16 @@ static struct cl_engine *reload_db(struct cl_engine *engine, unsigned int dbopti
     memset(&dbstat, 0, sizeof(struct cl_stat));
     if((retval = cl_statinidir(dbdir, &dbstat))) {
 	logg("!cl_statinidir() failed: %s\n", cl_strerror(retval));
-	*ret = 1;
 	if(settings)
 	    cl_engine_settings_free(settings);
-	return NULL;
+	return;
     }
 
     if(!(engine = cl_engine_new())) {
 	logg("!Can't initialize antivirus engine\n");
-	*ret = 1;
 	if(settings)
 	    cl_engine_settings_free(settings);
-	return NULL;
+	return;
     }
 
     if(settings) {
@@ -227,20 +213,20 @@ static struct cl_engine *reload_db(struct cl_engine *engine, unsigned int dbopti
     if((retval = cl_load(dbdir, engine, &sigs, dboptions))) {
 	logg("!reload db failed: %s\n", cl_strerror(retval));
 	cl_engine_free(engine);
-	*ret = 1;
-	return NULL;
+	return;
     }
 
     if((retval = cl_engine_compile(engine)) != 0) {
 	logg("!Database initialization error: can't compile engine: %s\n", cl_strerror(retval));
 	cl_engine_free(engine);
-	*ret = 1;
-	return NULL;
+	return;
     }
     logg("Database correctly reloaded (%u signatures)\n", sigs);
 
-    thrmgr_setactiveengine(engine);
-    return engine;
+    pthread_mutex_lock(&reload_mutex);
+    time(&reloaded_time);
+    reload_engine = engine;
+    pthread_mutex_unlock(&reload_mutex);
 }
 
 /*
@@ -713,6 +699,7 @@ int recvloop_th(int *socketds, unsigned nsockets, struct cl_engine *engine, unsi
 	unsigned long long val;
 	size_t i, j, rr_last = 0;
 	pthread_t accept_th;
+	pthread_t reload_th;
 	pthread_mutex_t fds_mutex = PTHREAD_MUTEX_INITIALIZER;
 	pthread_mutex_t recvfds_mutex = PTHREAD_MUTEX_INITIALIZER;
 	struct acceptdata acceptdata = ACCEPTDATA_INIT(&fds_mutex, &recvfds_mutex);
@@ -1347,10 +1334,20 @@ int recvloop_th(int *socketds, unsigned nsockets, struct cl_engine *engine, unsi
 	if(selfchk) {
 	    time(&current_time);
 	    if((current_time - start_time) >= (time_t)selfchk) {
-		if(reload_db(engine, dboptions, opts, TRUE, &ret)) {
+		if(!dbstat.entries) {
+		    logg("No stats for Database check - forcing reload\n");
+		    pthread_mutex_lock(&reload_mutex);
+		    reload = 1;
+		    pthread_mutex_unlock(&reload_mutex);
+		}
+
+		if(cl_statchkdir(&dbstat) == 1) {
+		    logg("SelfCheck: Database modification detected. Forcing reload.\n");
 		    pthread_mutex_lock(&reload_mutex);
 		    reload = 1;
 		    pthread_mutex_unlock(&reload_mutex);
+		} else {
+		    logg("SelfCheck: Database status OK.\n");
 		}
 		time(&start_time);
 	    }
@@ -1360,34 +1357,45 @@ int recvloop_th(int *socketds, unsigned nsockets, struct cl_engine *engine, unsi
 	pthread_mutex_lock(&reload_mutex);
 	if(reload) {
 	    pthread_mutex_unlock(&reload_mutex);
-#if defined(FANOTIFY) || defined(CLAMAUTH)
-	    if(optget(opts, "ScanOnAccess")->enabled && tharg) {
-		logg("Restarting on-access scan\n");
-		pthread_kill(fan_pid, SIGUSR1);
-		pthread_join(fan_pid, NULL);
-	    }
-#endif
-	    engine = reload_db(engine, dboptions, opts, FALSE, &ret);
-	    if(ret) {
-		logg("Terminating because of a fatal error.\n");
-		if(new_sd >= 0)
-		    closesocket(new_sd);
-		break;
-	    }
-
+	    struct reload_db_args reload_db_args = {
+		.engine = engine,
+		.dboptions = dboptions,
+		.opts = opts,
+	    };
 	    pthread_mutex_lock(&reload_mutex);
 	    reload = 0;
-	    time(&reloaded_time);
+	    pthread_create(&reload_th, NULL, reload_db, &reload_db_args);
 	    pthread_mutex_unlock(&reload_mutex);
+	} else {
+	    if(reload_engine != NULL) {
 #if defined(FANOTIFY) || defined(CLAMAUTH)
-	    if(optget(opts, "ScanOnAccess")->enabled && tharg) {
-		tharg->engine = engine;
-		pthread_create(&fan_pid, &fan_attr, fan_th, tharg);
-	    }
+		pthread_mutex_unlock(&reload_mutex);
+		if(optget(opts, "ScanOnAccess")->enabled && tharg) {
+		    logg("Restarting on-access scan\n");
+		    pthread_kill(fan_pid, SIGUSR1);
+		    pthread_join(fan_pid, NULL);
+		}
+		pthread_mutex_lock(&reload_mutex);
 #endif
-	    time(&start_time);
-	} else {
-	    pthread_mutex_unlock(&reload_mutex);
+		static struct cl_engine *tmp;
+		tmp = engine;
+		engine = reload_engine;
+		thrmgr_setactiveengine(engine);
+		time(&start_time);
+		reload_engine = NULL;
+		pthread_mutex_unlock(&reload_mutex);
+
+		cl_engine_free(tmp);
+
+#if defined(FANOTIFY) || defined(CLAMAUTH)
+		if(optget(opts, "ScanOnAccess")->enabled && tharg) {
+		    tharg->engine = engine;
+		    pthread_create(&fan_pid, &fan_attr, fan_th, tharg);
+		}
+#endif
+	    } else {
+		pthread_mutex_unlock(&reload_mutex);
+	    }
 	}
     }
 
diff --git a/clamd/server.h b/clamd/server.h
index bc2a296..e171ea9 100644
--- a/clamd/server.h
+++ b/clamd/server.h
@@ -46,6 +46,13 @@ struct thrwarg {
     unsigned int options;
 };
 
+/* reload_db arguments */
+struct reload_db_args {
+    struct cl_engine *engine;
+    unsigned int dboptions;
+    const struct optstruct *opts;
+};
+
 int recvloop_th(int *socketds, unsigned nsockets, struct cl_engine *engine, unsigned int dboptions, const struct optstruct *opts);
 void sighandler(int sig);
 void sighandler_th(int sig);
-- 
1.9.0-zedat

_______________________________________________
http://lurker.clamav.net/list/clamav-devel.html
Please submit your patches to our Bugzilla: http://bugs.clamav.net