Re: TSVNCache memory leaks

Stefan Fuhrmann wrote:
> Stefan Küng wrote:
>
>> Stefan Fuhrmann wrote:
>>> Hi there,
>>>
>>> some days ago, a coworker had TSVNCache go berserk and
>>> finally eating 3GB of memory (x64). That gave me sufficient
>>> motivation to search the code for improper memory management.
>>>
>>> r15974 fixes a number of obvious ones and I merged that code
>>> also into 1.6.x. r15975-7 try to make other parts more robust.
>>>
>>> Since TSVNCache is notoriously hard to test, it is very possible
>>> that I broke something. If you encounter any problem with the
>>> next nightlies, please tell us.
>>>
>>> Stefan, could you review my changes?
>> Had to change some of your code:
>> * didn't compile
>
> Oops .. I really want to apologize for that! I thought, I had compiled
> just before committing ... :(
>
>> * had to remove most of the auto_ptr use you introduced because they
>> threw runtime exceptions
>
> I finally figured out how to debug TSVNCache - it wasn't that
> hard after all. And I fixed all auto_ptr usage issues locally but
> will not commit them right row (see below).
>
>> * your 'paranoia' for-loop wouldn't actually do anything?
>
> It does. In particular, it will clear the whole string, if it is not
> 0-terminated. Just terminating it at the end of the buffer may
> make the result accidentally matching a valid path.
>
> In r15991, I enhanced my code a bit. It will now clear all
> bytes beyond the paths terminating 0. Also, it enables
> strict memory checking in debug mode.
>
>> But due to your changes, I found another bug which I could fix.
>
> And there is a lot more going on in TSVNCache:
>
> * tear-down sequence is broken: triggers debug exception
> in the CRWSection destructor.

That's not really a problem: the CRWSection is destroyed when the
process quits, and if you look at the code you'll find that the only
place we don't release the section is right at the end before the
process exits.

> * someone is writing to deleted data, possibly in
> CDirectoryWatcher::CloseInfoMap

I thought I fixed that just this afternoon?
It happened the WorkerThread(): if GetQueuedCompletionStatus() succeeds,
the pdi variable could still point to already deleted memory. That's why
I added the check whether the pdi data is still in the watchInfoMap.

But if you still get such debug exception, I guess then it's not fixed
yet :(

> Because of the latter, I hold off my changes until the root cause
> has been found. My problem is that this seems to be very dated
> code and is hard to understand. For instance CloseInfoMap takes
> a handle parameter and will remove the respective path from
> other collections. But the remaining entries will be deleted without
> such special handling (and there are often more than one entries).
> Why?

CloseInfoMap() releases all handles to all watched folders. This is for
example called when we get a 'eject' message for a drive.
But we don't want the watcher to forget which paths to watch! We only
want to close all handles, let the watcher start watching all paths
again, except those matching the hdev handle (which corresponds to the
path(s) we want to stop watching, e.g. as mentioned for the drive the
user wants to eject).

Stefan

-- 
       ___
  oo  // \\      "De Chelonian Mobile"
 (_,\/ \_/ \     TortoiseSVN
   \ \_/_\_/>    The coolest Interface to (Sub)Version Control
   /_/   \_\     http://tortoisesvn.net
------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=1473380
To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].