Re: [PATCH] Fix segfault in repo->wc copy

alan.wood_at_clear.net.nz wrote:
> On 27 Aug 2008 at 9:51, Hyrum K. Wright wrote:
>
>> alan.wood_at_clear.net.nz wrote:
>>> I have done the same tests with your patch and it seems to work the same as what I
>>> was using.
>>> I have attached my patch which does what you were thinking and doesn't wait for
>>> time stamps. It can't be that important to wait as none of the other error conditions
>>> wait for them.
>>>
>>> Alan
>>>
>>> On 26 Aug 2008 at 16:23, Hyrum K. Wright wrote:
>>>
>>>> alan.wood_at_clear.net.nz wrote:
>>>>> I have just traced a bug in the "svn copy URL_at_rev file" command.
>>>>> While following instructions in the svn-book to retrieve a deleted file
>>>>> svn copy svn://fred.ncs.local/ncs/truck/.../a.txt_at_211 a.txt
>>>>> <segmentation fault>
>>>>> This occurred on a Linux machine.
>>>>> I repeated the command from windows command line with the same result.
>>>>>
>>>>> tested on svn version 1.5.1(Linux and win32) and 1.5.x branch(win32)
>>>>>
>>>>> I have traced this through to libsvn_client: copy.c line 1455
>>>>>
>>>>> The return from svn_wc_add_repos_file2() is not checked with SVN_ERR().
>>>>>
>>>>> This fails when the repository name does not exactly match the
>>>>> repository name on the command line.
>>>>> In my case the Linux attempt was different as I used the machines
>>>>> actual name "fred" as opposed to "subversion.ncs.local" which the
>>>>> working copy was built from. On the win32 case I was using a
>>>>> file:///c:/... on the command line, but the working copy was checked
>>>>> out by TortoiseSVN and had file:///C:/... in the working copy.
>>>>>
>>>>> The segmentation fault actually occurs a few lines down when calling
>>>>> extend_wc_mergeinfo() with dst_entry==NULL.
>>>>>
>>>>> After changing to catch the error from the svn_wc_add_repos_file2()
>>>>> I do get an error message about the repositories having different
>>>>> roots which with careful inspection of C vs c may help the user to
>>>>> get it right next time. Would be nicer to to check the repository by
>>>>> uuid rather than name, then check the path within the repository as
>>>>> a string. Or maybe I missed something completely.
>>>> I haven't thoroughly tested this, but figured I'd throw it out here. Alan, if
>>>> you could check and see if this fixes your problem, that'd be great. The patch
>>>> is against trunk, but should apply cleanly against 1.5.x.
>>>>
>>>> I do wonder why we don't just wrap the call to svn_wc_add_repos_file2 with
>>>> SVN_ERR(). Is there a particular reason why we need to wait for timestamps
>>>> before returning the error?
>>>>
>>>> -Hyrum
>>>>
>>>> [[[
>>>> If we can't add the target of the copy to the working copy, don't continue
>>>> trying to calculate mergeinfo based upon the non-existent path. This fixes a
>>>> segfault reported here:
>>>> http://svn.haxx.se/dev/archive-2008-08/0667.shtml
>>>>
>>>> Found by: Alan Wood <alan.wood_at_clear.net.nz>
>>>>
>>>> * subversion/libsvn_client/copy.c
>>>> (repos_to_wc_copy_single): Don't continue with mergeinfo calculation if the
>>>> target of the copy couldn't be added to the working copy.
>>>> ]]]
>>>>
>>> Index: copy.c
>>> ===================================================================
>>> --- copy.c (revision 32704)
>>> +++ copy.c (working copy)
>>> @@ -1452,12 +1452,12 @@
>>>
>>> if (! SVN_IS_VALID_REVNUM(src_revnum))
>>> src_revnum = real_rev;
>>>
>>> - err = svn_wc_add_repos_file2
>>> + SVN_ERR(err = svn_wc_add_repos_file2
>>> (pair->dst, adm_access,
>>> new_text_path, NULL, new_props, NULL,
>>> same_repositories ? pair->src : NULL,
>>> same_repositories ? src_revnum : SVN_INVALID_REVNUM,
>>> - pool);
>>> + pool));
>>>
>>> SVN_ERR(svn_wc_entry(&dst_entry, pair->dst, adm_access, FALSE, pool));
>>> SVN_ERR(calculate_target_mergeinfo(ra_session, &src_mergeinfo,
>> The general idea looks sound, but I'd still like some feedback about the
>> sleeping for timestamps issue. In addition to the above, we should get rid for
>> the err variable, and the subsequent use of it in SVN_ERR(err) at the end of the
>> function.
>
> The alternative though is to make sure that the error returns from the calls to
> svn_wc_entry(), calculate_target_mergeinfo() & extend_wc_mergeinfo()
> also call svn_sleep_for_timestamps() on the way out of the function.
> Looking at the rest of copy.c there are examples of both with the
> handling of the sleep.
> My first reaction would be for the sleep to be done at places in
> the code where it is important for the normal flow and also done at
> the end of the public client routines in the case of an error. In
> our case the svn_sleep_for_timestamps() would be done in
> svn_client_move5() & svn_client_copy4() when (err).
> This would keep the error handling code as simple as possible.

True. I think I'd rather just wrap the call to svn_wc_add_repos_file2() with
SVN_ERR() and be done with it. If nobody objects (in a loud and obnoxious
fashion) than I shall commit a patch to that effect tomorrow.

-Hyrum