Re: [PATCH v2] remote: add get-url subcommand

From: Ben Boeckel <mathstuf@gmail.com>
Date: Aug 4, 2015, 00:16 UTC
Message-ID: <20150804001631.GA10416@megas.kitware.com>
In-Reply-To: <CAPig+cQO_jXZmJG=EccwQ408TiAk7_BiQF1UKA31phXF7CXy6w@mail.gmail.com>

On Mon, Aug 03, 2015 at 19:38:15 -0400, Eric Sunshine wrote:

Show 7 quoted lines

> On Mon, Aug 3, 2015 at 5:00 PM, Ben Boeckel <mathstuf@gmail.com> wrote:
> > +               OPT_BOOL('\0', "push", &push_mode,
> > +                        N_("query push URLs")),
> 
> A bit more explanatory:
> 
>     "query push URLs rather than fetch URLs"

Fixed.

Show 8 quoted lines

> > +               OPT_BOOL('\0', "all", &all_mode,
> > +                        N_("return all URLs")),
> > +               OPT_END()
> > +       };
> > +       argc = parse_options(argc, argv, NULL, options, builtin_remote_geturl_usage,
> > +                            PARSE_OPT_KEEP_ARGV0);
> 
> What is the reason for PARSE_OPT_KEEP_ARGV0 in this case?

Copied from get-url; I presume for more natural argv[] usage within the function.

> > +       if (argc < 1 || argc > 2)
> > +               usage_with_options(builtin_remote_geturl_usage, options);
> 
> So,  'argc' must be 1 or 2, which in 'argv' terms is argv[0] and argv[1]).

…

> > +       remotename = argv[1];
> 
> But here, argv[1] is accessed unconditionally, even though 'argc' may
> have been 1, thus out of bounds.

Yep, should be (argc < 2 || argc > 2) (or 1 if PARSE_OPT_KEEP_ARGV0 is removed). Off-by-one when converting from get-url.

I'll reroll tomorrow morning in case there are more comments until then (particularly about PARSE_OPT_KEEP_ARGV0).

Thanks,

--Ben