On Sat, Jan 29, 2011 at 8:20 AM, Bert Huijben <b...@qqmail.nl> wrote: > > >> -----Original Message----- >> From: Daniel Shahaf [mailto:d...@daniel.shahaf.name] >> Sent: zaterdag 29 januari 2011 4:24 >> To: dev@subversion.apache.org >> Cc: comm...@subversion.apache.org >> Subject: Re: svn commit: r1064847 - >> /subversion/trunk/subversion/svnserve/serve.c >> >> hwri...@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
