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
