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