Re: [PATCH v2 01/10] argp-parse.c (__argp_input): Don't crash if pstate is NULL

[Robbie Harwood]

Colin Watson <cjwatson@ubuntu.com> writes:

> On Tue, Dec 07, 2021 at 11:59:01AM -0500, Robbie Harwood wrote:
>> Bruno Haible <bruno@clisp.org> writes:
>> > I don't think this patch is needed, because:
>> >
>> > 1) The application cannot construct a 'struct argp_state' by itself, since 
>> > [1]
>> >    says that the 'struct argp_state' contains a member 'pstate' that is
>> >    "Private, for use by the argp implementation.".
>> >
>> > 2) The only place in the gnulib / glibc code where a 'struct argp_state' is
>> >    being constructed, is in function parser_init, invoked from 
>> > 'argp_parse',
>> >    and there a non-NULL value is assigned.
>> >
>> > In other words, there is no way, compliant with the documented API, that a
>> > NULL pointer can arise as state->pstate.
>> >
>> > Bruno
>> >
>> > [1] 
>> > https://www.gnu.org/software/libc/manual/html_node/Argp-Parsing-State.html
>> 
>> Thanks for taking a look.  I don't have more information on this one
>> beyond the little that's in the commit, so unless Colin remembers why
>> this was needed in 2011, I'll propose grub drop it at some point.
>
> The original problem was:
>
>   https://bugs.debian.org/612692
>
> At the time, __argp_help looked like this:
>
>   void __argp_help (const struct argp *argp, FILE *stream,
>                     unsigned flags, char *name)
>   {
>     struct argp_state state;
>     memset (&state, 0, sizeof state);
>     state.root_argp = argp;
>     _help (argp, &state, stream, flags, name);
>   }
>
> As a result it was possible at the time to encounter the case where
> state was non-NULL but state->pstate was NULL.  However, this seems to
> have been fixed differently some time ago:
>
>   https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=14a582531c
>
> So I think it's probably fine to drop this patch from GRUB.

Appreciate the follow-up!  That sounds reasonable to me.

Be well,
--Robbie

signature.asc
Description: PGP signature