[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c

From: Hyrum K Wright <hyrum_at_hyrumwright.org>
Date: Sat, 29 Jan 2011 07:52:52 -0600

On Fri, Jan 28, 2011 at 9:23 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> hwright_at_apache.org wrote on Fri, Jan 28, 2011 at 20:01:35 -0000:
>> Author: hwright
>> Date: Fri Jan 28 20:01:35 2011
>> New Revision: 1064847
>>
>> URL: http://svn.apache.org/viewvc?rev=1064847&view=rev
>> Log:
>> * subversion/svnserve/serve.c
>>   (log_cmd): Remove a useless check, and replace it with an assert instead.
>>
>> Modified:
>>     subversion/trunk/subversion/svnserve/serve.c
>>
>> Modified: subversion/trunk/subversion/svnserve/serve.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnserve/serve.c?rev=1064847&r1=1064846&r2=1064847&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/svnserve/serve.c (original)
>> +++ subversion/trunk/subversion/svnserve/serve.c Fri Jan 28 20:01:35 2011
>> @@ -2008,18 +2008,17 @@ static svn_error_t *log_cmd(svn_ra_svn_c
>>      revprops = NULL;
>>    else if (strcmp(revprop_word, "revprops") == 0)
>>      {
>> +      SVN_ERR_ASSERT(revprop_items);
>> +
>> -      if (revprop_items)
>
> <as far as I can tell>
>
> The 'protocol' document explicitly allows the tuple to terminate
> immediately after the 'revprops' word --- which, is the case where the
> assert would fire; therefore, either your new check violates the
> documented protocol, or the protocol documentation needs to be fixed.
>
> </as far as I can tell>
>
> Makes sense?

I guess.

The diff doesn't give much context, but the prior code was:

      revprops = apr_array_make(pool, revprop_items->nelts,
                                sizeof(char *));
      if (revprop_items)
        {
            ...
        }

Do you see the problem? The first statement *assumes* that
revprop_items isn't NULL (since it dereferences it), and then goes on
to check this same fact. In practice, I don't think we're ever see
this segfault, but it's a possibility (and may even be a remove DoS
bug if triggered correctly).

r1064847 doesn't change the fact that misbehavior still crashes the
server; it just makes that fact a bit more obvious. Improvements are
welcome.

-Hyrum
Received on 2011-01-29 14:53:38 CET

This is an archived mail posted to the Subversion Dev mailing list.