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

Re: svn commit: r1140861 - /subversion/trunk/subversion/svnadmin/main.c

From: Hyrum K Wright <hyrum_at_hyrumwright.org>
Date: Wed, 29 Jun 2011 10:10:08 -0500

On Wed, Jun 29, 2011 at 3:26 AM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> On Wed, 2011-06-29 at 02:16 +0300, Daniel Shahaf wrote:
>> hwright_at_apache.org wrote on Tue, Jun 28, 2011 at 21:47:06 -0000:
>> > Author: hwright
>> > Date: Tue Jun 28 21:47:06 2011
>> > New Revision: 1140861
>> >
>> > URL: http://svn.apache.org/viewvc?rev=1140861&view=rev
>> > Log:
>> > * subversion/svnadmin/main.c
>> >   (parse_args): Fix a theoretical bug by ensure we only attempt to parse args
>> >     if we think we have them.
>> >
>> > Modified:
>> >     subversion/trunk/subversion/svnadmin/main.c
>> >
>> > Modified: subversion/trunk/subversion/svnadmin/main.c
>> > URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnadmin/main.c?rev=1140861&r1=1140860&r2=1140861&view=diff
>> > ==============================================================================
>> > --- subversion/trunk/subversion/svnadmin/main.c (original)
>> > +++ subversion/trunk/subversion/svnadmin/main.c Tue Jun 28 21:47:06 2011
>> > @@ -559,9 +559,11 @@ parse_args(apr_array_header_t **args,
>> >    if (args)
>> >      {
>> >        *args = apr_array_make(pool, num_args, sizeof(const char *));
>> > -      while (os->ind < os->argc)
>> > -        APR_ARRAY_PUSH(*args, const char *) =
>> > -          apr_pstrdup(pool, os->argv[os->ind++]);
>> > +
>> > +      if (num_args)
>> > +        while (os->ind < os->argc)
>> > +          APR_ARRAY_PUSH(*args, const char *) =
>> > +            apr_pstrdup(pool, os->argv[os->ind++]);
>> >      }
>> >
>>
>> I don't understand this change.  The loop would execute zero times when
>> os->ind == os->argc, so what are you guarding against?
>>
>> I think you forgot to either:
>>
>> * check 'os != NULL'
>>
>> * move the apr_array_make() inside the check that NUM_ARGS > 0
>
> Actually, Hyrum's code looks correct to me.  It guards against 'os'
> because 'num_args' is zero if 'os' is null:
>
>  int num_args = os ? (os->argc - os->ind) : 0;
>
> And the array needs to be allocated if the caller requested it, even if
> it is going to have no elements.

Correct, that was my logic (and you explained it better than I would have).

-Hyrum
Received on 2011-06-29 17:10:44 CEST

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.