On 11.03.2016 15:55, Ivan Zhakov wrote:
> On 10 March 2016 at 21:14, Stefan Küng <tortoisesvn_at_gmail.com> 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_at_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.
Yes, that's the reason.
I can now reproduce the crash as well.
> 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.
Patch looks good!
Please commit.
But while I was going through the code and already made some similar
changes as in your patch I got an idea:
maybe we could remove the separate handling of logs with/without merge
info completely and just use one code path for all situations?
This would mean getting rid of the call to InternalLogWithMerge() and
call InternalLog() instead. Then we could also remove the CMergeLogger()
class.
I'm doing some testing with that change (not finished yet) soon.
But in the mean time: what do you think?
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=3165397
To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].
Received on 2016-03-11 18:40:24 CET