On Sat, Jan 29, 2011 at 8:20 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
>> Sent: zaterdag 29 januari 2011 4:24
>> To: dev_at_subversion.apache.org
>> Cc: commits_at_subversion.apache.org
>> Subject: Re: svn commit: r1064847 -
>> /subversion/trunk/subversion/svnserve/serve.c
>>
>> 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>
>
> And an assertion crashes svnserve, so I would recommend not creating network
> triggerable assertions (in non-maintainer builds) anyway.
Understood. But I didn't create the crash; it was already there as a
potential NULL pointer dereference. The same conditions which would
cause the new assert to fail would have led to a segfault in the old
code. Functionality-wise, I neither improved nor worsened the
situation. My goal was to illuminate the potential crash; I'd say
that's working just fine. :)
(And yes, I do think a more permanent fix is needed, which could be as
simple as returning an MALFORMED error. I haven't thought too much
about it.)
-Hyrum
Received on 2011-01-29 15:28:30 CET