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

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.

Also, per our patch guidelines[1], could you attach the next version of the
patch as text/x-diff, text/x-patch, or text/plain? This one was encoded as
Application/Octet-stream, which made it difficult to review.

Thanks,
-Hyrum

[1] http://subversion.tigris.org/hacking.html#patches