Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Julius Plenz]

Hi,

* Julius Plenz pl...@cis.fu-berlin.de [2014-04-07 20:43]:
  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.

We have been running ClamAV on all incoming mail for the past week
while periodically reloading a fresh definition database, and it works
very well. Any chance of getting this included into upstream?

Current patch against Git tag clamav-0.98.1 pasted below.

Cheers,

Julius
-- 
 clamd/server-th.c | 145 --
 clamd/server.h|   7 +++
 2 files changed, 94 insertions(+), 58 deletions(-)

diff --git a/clamd/server-th.c b/clamd/server-th.c
index 86a3a48..3df5c9c 100644
--- a/clamd/server-th.c
+++ b/clamd/server-th.c
@@ -64,6 +64,8 @@
 int progexit = 0;
 pthread_mutex_t exit_mutex = PTHREAD_MUTEX_INITIALIZER;
 int reload = 0;
+volatile int reload_in_progress = 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 +160,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;
-
-*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;
-   }
-}
-
-/* release old structure */
-if(engine) {
+const char *dbdir;
+int retval;
+unsigned int sigs = 0;
+struct cl_settings *settings = NULL;
+struct cl_engine *engine = 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;
+
+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 +190,12 @@ 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;
+   goto err;
 }
 
 if(!(engine = cl_engine_new())) {
logg(!Can't initialize antivirus engine\n);
-   *ret = 1;
-   if(settings)
-   cl_engine_settings_free(settings);
-   return NULL;
+   goto err;
 }
 
 if(settings) {
@@ -222,25 +205,38 @@ static struct cl_engine *reload_db(struct cl_engine 
*engine, unsigned int dbopti
logg(^Using default engine settings\n);
}
cl_engine_settings_free(settings);
+   settings = NULL;
 }
 
 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;
+   goto err;
 }
 
 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;
+   goto err;
 }
 logg(Database correctly reloaded (%u signatures)\n, sigs);
 
-thrmgr_setactiveengine(engine);
-return engine;
+pthread_mutex_lock(reload_mutex);
+time(reloaded_time);
+reload_engine = engine;
+goto end;
+
+err:
+if(settings)
+   cl_engine_settings_free(settings);
+if(engine)
+   cl_engine_free(engine);
+
+pthread_mutex_lock(reload_mutex);
+
+end:
+reload_in_progress = 0;
+pthread_mutex_unlock(reload_mutex);
+
+free(args);
 }
 
 /*
@@ -713,6 +709,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 

Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Steven Morgan]

Thanks for sending a patch, we will look at integrating it with ClamAV.

I have opened bug # 10979 at bugzilla.clamav.net for tracking.


On Mon, Apr 28, 2014 at 10:29 AM, Julius Plenz pl...@cis.fu-berlin.dewrote:

 Hi,

 * Julius Plenz pl...@cis.fu-berlin.de [2014-04-07 20:43]:
   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.

 We have been running ClamAV on all incoming mail for the past week
 while periodically reloading a fresh definition database, and it works
 very well. Any chance of getting this included into upstream?

 Current patch against Git tag clamav-0.98.1 pasted below.

 Cheers,

 Julius
 --
  clamd/server-th.c | 145
 --
  clamd/server.h|   7 +++
  2 files changed, 94 insertions(+), 58 deletions(-)

 diff --git a/clamd/server-th.c b/clamd/server-th.c
 index 86a3a48..3df5c9c 100644
 --- a/clamd/server-th.c
 +++ b/clamd/server-th.c
 @@ -64,6 +64,8 @@
  int progexit = 0;
  pthread_mutex_t exit_mutex = PTHREAD_MUTEX_INITIALIZER;
  int reload = 0;
 +volatile int reload_in_progress = 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 +160,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;
 -
 -*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;
 -   }
 -}
 -
 -/* release old structure */
 -if(engine) {
 +const char *dbdir;
 +int retval;
 +unsigned int sigs = 0;
 +struct cl_settings *settings = NULL;
 +struct cl_engine *engine = 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;
 +
 +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 +190,12 @@ 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;
 +   goto err;
  }

  if(!(engine = cl_engine_new())) {
 logg(!Can't initialize antivirus engine\n);
 -   *ret = 1;
 -   if(settings)
 -   cl_engine_settings_free(settings);
 -   return NULL;
 +   goto err;
  }

  if(settings) {
 @@ -222,25 +205,38 @@ static struct cl_engine *reload_db(struct cl_engine
 *engine, unsigned int dbopti
 logg(^Using default engine settings\n);
 }
 cl_engine_settings_free(settings);
 +   settings = NULL;
  }

  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;
 +   goto err;
  }

  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;
 +   goto err;
  }
  logg(Database correctly reloaded (%u signatures)\n, sigs);

 -thrmgr_setactiveengine(engine);
 -return engine;
 +pthread_mutex_lock(reload_mutex);
 +time(reloaded_time);
 +reload_engine = engine;
 +goto end;
 +
 +err:
 +if(settings)
 +   cl_engine_settings_free(settings);
 +if(engine)
 +   cl_engine_free(engine);
 +
 +pthread_mutex_lock(reload_mutex);
 +
 +end:
 +

Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Julius Plenz]

Hi, Mark!

* Mark Pizzolato - ClamAV-devel clamav-de...@subscriptions.pizzolato.net 
[2014-04-10 18:45]:
 One more question (as I originally said, I haven't looked closely at
 either the current code or your suggested patch)...  What happens if
 multiple reload requests come in right after each other while
 scanning thread(s) are still scanning some file(s)?

Then the threads would override each other's work and you'd have
unreferenced engines flying around. Good catch. Pasted below is a
patch to address that problem.

Julius
-- 
From a178a359970d65a0c439c8e3a84e861cbc9fd944 Mon Sep 17 00:00:00 2001
From: Julius Plenz pl...@cis.fu-berlin.de
Date: Mon, 14 Apr 2014 11:05:06 +0200
Subject: [PATCH] Introduce a reload_in_progress flag

---
 clamd/server-th.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/clamd/server-th.c b/clamd/server-th.c
index 9afa2b7..2167475 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;
+volatile int reload_in_progress = 0;
 struct cl_engine *reload_engine = NULL;
 time_t reloaded_time = 0;
 pthread_mutex_t reload_mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -226,6 +227,7 @@ static void *reload_db(void *argp)
 pthread_mutex_lock(reload_mutex);
 time(reloaded_time);
 reload_engine = engine;
+reload_in_progress = 0;
 pthread_mutex_unlock(reload_mutex);
 }
 
@@ -1355,7 +1357,11 @@ int recvloop_th(int *socketds, unsigned nsockets, struct 
cl_engine *engine, unsi
 
/* DB reload */
pthread_mutex_lock(reload_mutex);
-   if(reload) {
+   if(reload  reload_in_progress) {
+   logg(Database reload already in progress; ignoring reload 
request.\n);
+   reload = 0;
+   pthread_mutex_unlock(reload_mutex);
+   } else if(reload) {
pthread_mutex_unlock(reload_mutex);
struct reload_db_args reload_db_args = {
.engine = engine,
@@ -1364,6 +1370,7 @@ int recvloop_th(int *socketds, unsigned nsockets, struct 
cl_engine *engine, unsi
};
pthread_mutex_lock(reload_mutex);
reload = 0;
+   reload_in_progress = 1;
pthread_create(reload_th, NULL, reload_db, reload_db_args);
pthread_mutex_unlock(reload_mutex);
} else {
-- 
1.9.0-zedat

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

Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Julius Plenz]

Hi, Mark!

* Mark Pizzolato - ClamAV-devel clamav-de...@subscriptions.pizzolato.net 
[2014-04-08 00:02]:
  It appears that for every connection that is acceptey by clamd,
  the current engine value is passed in the conn struct. The
  engine struct has a ref count, and a process grabs the engine by
  calling cl_engine_addref(), thus increasing the ref count. Only
  when cl_engine_free() is called and the ref count is zero is the
  object actually freed.

 It would seem that there is a race condition in this paradigm.  The
 reference to the engine object should be added when the engine value
 is set in the conn structure (this determining of the engine value
 AND the addition of the reference count should be done with the
 related mutex held).

True, this would be the better approach. The downside is that one has
to free (i.e. decrease the ref count of) this not-yet-used object
every time an error occurs. So in many error cases, you'd have
useless calls to cl_engine_free().

 The current paradigm seems to be creating an un-accounted reference
 and later on incrementing the reference value.  By the time that
 increment happens the engine value which was passed may have already
 been freed and thus the pointer being deference is no longer
 pointing at a valid object.

This is a valid concern, but with the patch I posted I think this can
not happen. The patch mainly changes the behaviour of recvloop_th(),
the receiving thread. Further we have an accept thread and scanner
threads.

The receiving thread loops over outstanding requests and dispatches
them. It is after the dispatching is done that we check for errors, if
the program should exit, or if the DB should be reloaded. Then the
loop is re-started.

The dispatching happens via the following callpath:

recvloop_th - parse_dispatch_cmd -
- execute_or_dispatch_command - dispatch_command

dispatch_command() duplicates the conn structure, and now calls
cl_engine_addref(dup_conn-engine). But only now the connection is
handed to a scanner thread via thrmgr_group_dispatch(). In case this
works, dispatch_command() will not free the engine, because this is
now the scanner thread's job. The function returns to the receive loop
eventually -- only now we can come to the point where we re-set the
pointer to the newly-loaded engine and free the old one.

So I think there is no race condition here.

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

Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Mark Pizzolato - ClamAV-devel]

Hi Julius,

On Thursday, April 10, 2014 at 4:19 AM, Julius Plenz wrote:
 Hi, Mark!
 
 * Mark Pizzolato - ClamAV-devel clamav-de...@subscriptions.pizzolato.net 
 [2014-04-08 00:02]:
   It appears that for every connection that is acceptey by clamd, the
   current engine value is passed in the conn struct. The engine
   struct has a ref count, and a process grabs the engine by calling
   cl_engine_addref(), thus increasing the ref count. Only when
   cl_engine_free() is called and the ref count is zero is the object
   actually freed.
 
  It would seem that there is a race condition in this paradigm.  The
  reference to the engine object should be added when the engine value
  is set in the conn structure (this determining of the engine value AND
  the addition of the reference count should be done with the related
  mutex held).
 
 True, this would be the better approach. The downside is that one has to
 free (i.e. decrease the ref count of) this not-yet-used object every time an
 error occurs. So in many error cases, you'd have useless calls to
 cl_engine_free().
 
  The current paradigm seems to be creating an un-accounted reference
  and later on incrementing the reference value.  By the time that
  increment happens the engine value which was passed may have already
  been freed and thus the pointer being deference is no longer pointing
  at a valid object.
 
 This is a valid concern, but with the patch I posted I think this can not 
 happen.
 The patch mainly changes the behaviour of recvloop_th(), the receiving
 thread. Further we have an accept thread and scanner threads.
 
 The receiving thread loops over outstanding requests and dispatches them.
 It is after the dispatching is done that we check for errors, if the program
 should exit, or if the DB should be reloaded. Then the loop is re-started.
 
 The dispatching happens via the following callpath:
 
 recvloop_th - parse_dispatch_cmd -
 - execute_or_dispatch_command - dispatch_command
 
 dispatch_command() duplicates the conn structure, and now calls
 cl_engine_addref(dup_conn-engine). But only now the connection is
 handed to a scanner thread via thrmgr_group_dispatch(). In case this works,
 dispatch_command() will not free the engine, because this is now the
 scanner thread's job. The function returns to the receive loop eventually --
 only now we can come to the point where we re-set the pointer to the
 newly-loaded engine and free the old one.
 
 So I think there is no race condition here.

Well, I initially said I haven't looked closely'.  You now have looked closely 
and your analysis looks quite reasonable.

One more question (as I originally said, I haven't looked closely at either the 
current code or your suggested patch)...  What happens if multiple reload 
requests come in right after each other while scanning thread(s) are still 
scanning some file(s)?

- Mark

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

Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Julius Plenz]

Hi, Mark!

* Mark Pizzolato - ClamAV-devel clamav-de...@subscriptions.pizzolato.net 
[2014-04-08 00:02]:
  It appears that for every connection that is acceptey by clamd,
  the current engine value is passed in the conn struct. The
  engine struct has a ref count, and a process grabs the engine by
  calling cl_engine_addref(), thus increasing the ref count. Only
  when cl_engine_free() is called and the ref count is zero is the
  object actually freed.

 It would seem that there is a race condition in this paradigm.  The
 reference to the engine object should be added when the engine value
 is set in the conn structure (this determining of the engine value
 AND the addition of the reference count should be done with the
 related mutex held).  The current paradigm seems to be creating an
 un-accounted reference and later on incrementing the reference
 value.  By the time that increment happens the engine value which
 was passed may have already been freed and thus the pointer being
 deference is no longer pointing at a valid object.

Yes, you are certainly right. Thanks for pointing this out! I will try
to work around this issue some time this week.

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

Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Julius Plenz]

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 pl...@cis.fu-berlin.de
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 = 

Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Mark Pizzolato - ClamAV-devel]

  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?

Hi,

I haven't looked closely, but how is the fact that each thread (which may 
currently be scanning a different file and may finish at some arbitrary time in 
the future) has a reference to the current engine object handled?  It would 
seem that some sort of a reference count could be used to manage which thread 
actually frees the engine object which is being replaced...  Maybe the settings 
object also needs a local reference in each of the scanning threads...

- Mark Pizzolato


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

Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Julius Plenz]

Hi, Mark!

* Mark Pizzolato - ClamAV-devel clamav-de...@subscriptions.pizzolato.net 
[2014-04-07 21:17]:
 I haven't looked closely, but how is the fact that each thread
 (which may currently be scanning a different file and may finish at
 some arbitrary time in the future) has a reference to the current
 engine object handled?

It appears that for every connection that is acceptey by clamd, the
current engine value is passed in the conn struct. The engine
struct has a ref count, and a process grabs the engine by calling
cl_engine_addref(), thus increasing the ref count. Only when
cl_engine_free() is called and the ref count is zero is the object
actually freed.

This is what I got from a cursory reading of the code... (such a
request/command is passed around quite a lot.) I verified that the old
engine was indeed freed after the scan finished by plucking in debug
statements in the relevant functions.

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

Re: [Clamav-devel] clamd: Keep scanning while reloading database

[Mark Pizzolato - ClamAV-devel]

Hi Julius,

On Monday, April 07, 2014 at 1:50 PM, Julius Plenz wrote:
 * Mark Pizzolato - ClamAV-devel clamav-
 de...@subscriptions.pizzolato.net [2014-04-07 21:17]:
  I haven't looked closely, but how is the fact that each thread (which
  may currently be scanning a different file and may finish at some
  arbitrary time in the future) has a reference to the current engine
  object handled?
 
 It appears that for every connection that is acceptey by clamd, the current
 engine value is passed in the conn struct. The engine struct has a ref
 count, and a process grabs the engine by calling cl_engine_addref(), thus
 increasing the ref count. Only when
 cl_engine_free() is called and the ref count is zero is the object actually
 freed.

It would seem that there is a race condition in this paradigm.  The reference 
to the engine object should be added when the engine value is set in the conn 
structure (this determining of the engine value AND the addition of the 
reference count should be done with the related mutex held).  The current 
paradigm seems to be creating an un-accounted reference and later on 
incrementing the reference value.  By the time that increment happens the 
engine value which was passed may have already been freed and thus the pointer 
being deference is no longer pointing at a valid object.

 This is what I got from a cursory reading of the code... (such a
 request/command is passed around quite a lot.) I verified that the old engine
 was indeed freed after the scan finished by plucking in debug statements in
 the relevant functions.

It is good that it usually is freed.  That doesn't prove that there isn't a 
race condition which can cause corruption at arbitrary times (Not your fault).

- Mark


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