[Clamav-devel] clamd: Keep scanning while reloading database
plenz at cis.fu-berlin.de
Thu Apr 10 07:19:27 EDT 2014
* Mark Pizzolato - ClamAV-devel <clamav-devel at 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
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.
More information about the clamav-devel