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
