Re: clamd: Keep scanning while reloading database

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 <[email protected]>wrote:

> Hi,
>
> * Julius Plenz <[email protected]> [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 fds_mutex = PTHREAD_MUTEX_INITIALIZER;
>         pthread_mutex_t recvfds_mutex = PTHREAD_MUTEX_INITIALIZER;
>         struct acceptdata acceptdata = ACCEPTDATA_INIT(&fds_mutex,
> &recvfds_mutex);
> @@ -1347,10 +1344,19 @@ 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);
> +               }
> +               else 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);
>             }
> @@ -1358,34 +1364,57 @@ 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;
> +           reload_db_args = (struct reload_db_args *)
> malloc(sizeof(struct reload_db_args));
> +           if (reload_db_args)
> +           {
> +               reload_db_args->engine = engine;
> +               reload_db_args->dboptions = dboptions;
> +               reload_db_args->opts = opts;
> +
> +               pthread_mutex_lock(&reload_mutex);
> +               reload = 0;
> +               reload_in_progress = 1;
> +
> +               if (pthread_create(&reload_th, NULL, reload_db,
> reload_db_args) != 0) {
> +                   logg("*Error dispatching DB reload thread!\n");
> +                   reload_in_progress = 0;
> +                   free(reload_db_args);
> +               }
> +               pthread_mutex_unlock(&reload_mutex);
> +           }
> +       } else if(reload_engine != NULL) {
>  #if defined(FANOTIFY) || defined(CLAMAUTH)
> +           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);
>             }
> -#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;
> -           }
> -
>             pthread_mutex_lock(&reload_mutex);
> -           reload = 0;
> -           time(&reloaded_time);
> +#endif
> +           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
> -           time(&start_time);
>         } 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
>
_______________________________________________
http://lurker.clamav.net/list/clamav-devel.html
Please submit your patches to our Bugzilla: http://bugs.clamav.net