Re: Log caching: Crash in `Get merge logs' without access to the repository root

On 10 March 2016 at 21:14, Stefan Küng <[email protected]> wrote:
> On 09.03.2016 15:24, Evgeny Kotkov wrote:
>> The `Get merge logs' command can cause a segfault in TortoiseSVN 1.9.3.
>>
>> The crash is also reproducible in trunk@r27234 and is caused by an access
>> violation (out of bounds access in an std::vector) in the implementation of
>> the log caching.  Please note that it should be reproducible irrespectively
>> of whether the log caching feature is enabled or disabled in the settings.
>>
>> Reproduction script:
>>
>>   1. Create a repository with a /trunk folder hosted by httpd
>>   2. Prohibit access to the root of the repository
>>   3. Allow rw access to /trunk
>>   4. In the TortoiseSVN log viewer, click `Get merge logs' for /trunk or,
>>      alternatively, run the following command:
>>      TortoiseProc /command:log /path:"http://localhost/repository/trunk" /merge
>
> No crash for me with a nightly build.
> And I also don't get a crash with 1.9.3 x64.
> Testing against a VisualSVN server 3.5, both with an existing repo and a
> completely new one with only revision 1 where trunk/branches/tags was
> created.
>
> I denied access to root (verified using the repo browser) but allowed rw
> access to trunk/branches/tags. Showed the log for trunk, clicked
> "include merged revisions", and I also tried with the command line.
>
> no crash in all tests.
>

I can confirm the crash with the development build from trunk
(however, I didn't try 1.9.3). Perhaps, the reason why it didn't crash
in your tests is that you first opened log without merged revisions
and only then requested the merged revisions. At this point, the
information about the revisions has already been cached.

Currently, the merge-aware log in TortoiseSVN works like this:

1. The first request it does is like 'svn log -g', but without the
information about the author, log message and without the changed path
list.
2. Then, per every received revision info, it checks if the data for
this revision is available in the log cache.
   a) If it's not, another request is performed that asks the
information for 100 revisions starting from the interesting one. This
request is done for the repository root.
3. The log information is delivered straight from the log cache.

In the described case, step 2a) fails since the user cannot access the
repository root.

I propose to fix the issue by also asking for author and log message
during step 1, because:

- Server side processing for merge-aware logs already requires the server to
  read the changed path list, so processing time should be about the same.
- Transferring log messages over the network should be fast, but
opening a huge number
  of connections and making a lot of unnecessary requests can be slow,
especially for
  high-latency networks.

Please see the attached patch.

-- 
Ivan Zhakov

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3165379

To unsubscribe from this discussion, e-mail: [[email protected]].

Index: src/LogCache/CacheLogQuery.cpp
===================================================================
--- src/LogCache/CacheLogQuery.cpp	(revision 27234)
+++ src/LogCache/CacheLogQuery.cpp	(working copy)
@@ -1,6 +1,6 @@
 // TortoiseSVN - a Windows shell extension for easy version control
 
-// Copyright (C) 2007-2015 - TortoiseSVN
+// Copyright (C) 2007-2016 - TortoiseSVN
 
 // This program is free software; you can redistribute it and/or
 // modify it under the terms of the GNU General Public License
@@ -355,6 +355,19 @@
 
         receiverError = false;
 
+        if (rev == SVN_INVALID_REVNUM)
+        {
+            if (options.GetReceiver() != NULL)
+            {
+                options.GetReceiver()->ReceiveLog(changes
+                    , rev
+                    , stdRevProps
+                    , userRevProps
+                    , mergeInfo);
+            }
+            return;
+        }
+
         // one entry more that we received
 
         ++receiveCount;
@@ -522,7 +535,7 @@
                       , options.GetStrictNodeHistory()
                       , this
                       , true
-                      , false
+                      , options.GetIncludeMerges()
                       , options.GetIncludeStandardRevProps()
                       , options.GetIncludeUserRevProps()
                       , TRevPropNames());
@@ -1091,32 +1104,18 @@
 
     ResetObjectTranslations();
 
-    // this object will only receive the revision numbers
-    // and give us a callback to add the other info from cache
-    // (auto-fill the latter)
-
-    CMergeLogger logger (this, options);
-
-    // fetch revisions only but include merge children
-
-    CTSVNPath path;
-    if (startPath.IsRoot())
-        path.SetFromSVN (URL);
-    else
-        path.SetFromSVN (URL + startPath.GetPath().c_str());
-
-    svnQuery->Log ( CTSVNPathList (path)
-                  , static_cast<long>(startRevision)
-                  , static_cast<long>(startRevision)
-                  , static_cast<long>(endRevision)
-                  , limit
-                  , options.GetStrictNodeHistory()
-                  , &logger
-                  , false
-                  , true
-                  , false
-                  , false
-                  , TRevPropNames());
+    // Request all interesting revisions: we cannot use log caching for
+    // merge logs since some of paths 
+    CLogFiller(repositoryInfoCache)
+        .FillLog(cache
+            , URL
+            , uuid
+            , svnQuery
+            , startRevision
+            , endRevision
+            , startPath
+            , limit
+            , options);
 }
 
 // follow copy history until the startRevision is reached