[PATCH]Fix Segfault while detecting the implicit merge source

[Kamesh Jayachandran <ka...@collab.net>]

Hi All,
Currently 'svn merge' from a WC which has no copy_source segfaults.

The attached patch fixes it.

With regards
Kamesh Jayachandran

Re: [PATCH]Fix Segfault while detecting the implicit merge source

[Kamesh Jayachandran <ka...@collab.net>]

Daniel Rall wrote:
> On Tue, 15 May 2007, Kamesh Jayachandran wrote:
>
>   
>>>>                     ctx, pool));
>>>> @@ -241,7 +242,7 @@
>>>>     {
>>>>       const char *path;
>>>>       apr_hash_this(hi, (void *) &path, NULL, NULL);
>>>> -      if (strcmp(path, copyfrom_path) != 0)
>>>> +      if (copyfrom_path && strcmp(path, copyfrom_path) != 0)
>>>>    
>>>>         
>>> Shouldn't this be?:
>>>
>>>        if (copyfrom_path == NULL || strcmp(path, copyfrom_path) != 0)
>>>
>>>       
>> No. It segfaults in strcmp when copyfrom_path is NULL.
>>     
>
> Right -- and the additional check avoids the strcmp() in the case
> where copyfrom_path is NULL.  However, the strcmp() is only there to
> weed out the case where copyfrom_path exists as a merge source.  If we
> have no copyfrom_path, we can avoid the string equality check
> entirely.
>
> +1 to commit with this tweak.
>   

Thanks Committed in r25024.

With regards
Kamesh Jayachandran

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH]Fix Segfault while detecting the implicit merge source

[Daniel Rall <dl...@collab.net>]

On Tue, 15 May 2007, Kamesh Jayachandran wrote:

> 
> >>                     ctx, pool));
> >>@@ -241,7 +242,7 @@
> >>     {
> >>       const char *path;
> >>       apr_hash_this(hi, (void *) &path, NULL, NULL);
> >>-      if (strcmp(path, copyfrom_path) != 0)
> >>+      if (copyfrom_path && strcmp(path, copyfrom_path) != 0)
> >>    
> >
> >Shouldn't this be?:
> >
> >        if (copyfrom_path == NULL || strcmp(path, copyfrom_path) != 0)
> >
>
> No. It segfaults in strcmp when copyfrom_path is NULL.

Right -- and the additional check avoids the strcmp() in the case
where copyfrom_path is NULL.  However, the strcmp() is only there to
weed out the case where copyfrom_path exists as a merge source.  If we
have no copyfrom_path, we can avoid the string equality check
entirely.

+1 to commit with this tweak.

Re: [PATCH]Fix Segfault while detecting the implicit merge source

[Kamesh Jayachandran <ka...@collab.net>]

>>                      ctx, pool));
>> @@ -241,7 +242,7 @@
>>      {
>>        const char *path;
>>        apr_hash_this(hi, (void *) &path, NULL, NULL);
>> -      if (strcmp(path, copyfrom_path) != 0)
>> +      if (copyfrom_path && strcmp(path, copyfrom_path) != 0)
>>     
>
> Shouldn't this be?:
>
>         if (copyfrom_path == NULL || strcmp(path, copyfrom_path) != 0)
>
>   
No. It segfaults in strcmp when copyfrom_path is NULL.

With regards
Kamesh Jayachandran

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH]Fix Segfault while detecting the implicit merge source

[Daniel Rall <dl...@collab.net>]

On Tue, 15 May 2007, Kamesh Jayachandran wrote:

> Currently 'svn merge' from a WC which has no copy_source segfaults.

Nice catch, Kamesh.  A question below...

> [[[
> Fix segfault on svn merge source detection.
> 
> * subversion/libsvn_client/log.c
>   (svn_client__suggest_merge_sources): 
>    Don't assume copy source to exist always.
> 
> Patch by: kameshj
> ]]]

> Index: subversion/libsvn_client/log.c
> ===================================================================
> --- subversion/libsvn_client/log.c	(revision 25012)
> +++ subversion/libsvn_client/log.c	(working copy)
> @@ -233,7 +233,8 @@
>    /* ### TODO: Use RA APIs directly to improve efficiency. */
>    SVN_ERR(svn_client__get_copy_source(path_or_url, revision, &copyfrom_path,
>                                        &copyfrom_rev, ctx, pool));
> -  APR_ARRAY_PUSH(*suggestions, const char *) = copyfrom_path;
> +  if (copyfrom_path)
> +    APR_ARRAY_PUSH(*suggestions, const char *) = copyfrom_path;
>  
>    SVN_ERR(svn_client_get_mergeinfo(&mergeinfo, path_or_url, revision,
>                                     ctx, pool));
> @@ -241,7 +242,7 @@
>      {
>        const char *path;
>        apr_hash_this(hi, (void *) &path, NULL, NULL);
> -      if (strcmp(path, copyfrom_path) != 0)
> +      if (copyfrom_path && strcmp(path, copyfrom_path) != 0)

Shouldn't this be?:

        if (copyfrom_path == NULL || strcmp(path, copyfrom_path) != 0)

>          {
>            APR_ARRAY_PUSH(*suggestions, const char *) = apr_pstrdup(pool, path);
>          }