Re: [musl] [PATCH] fix error handling in getsubopt()

[Rich Felker <dalias@libc.org>]

Thanks for sending this to the list.

On Wed, Nov 03, 2021 at 01:47:57PM +0100, Marian Buschsieweke wrote:
> The man page of getsubopt says
> 
> >    int getsubopt(char **restrict optionp, char *const *restrict tokens,
> >                  char **restrict valuep);
> >
> > [...]
> > RETURN VALUE
> >       If the first suboption in optionp is recognized, getsubopt()
> >       returns the index of the matching suboption element in tokens.
> >       Otherwise, -1 is returned and *valuep is the entire name[=value]
> >       string.

I went looking to replace this with a citation for the actual
specification, since the man page is not the specification for
standards-specified functions, and found that the above seems to be a
glibc extension, and it's not clear whether it meets the requirements
of the spec. On detailed reading, it's not even clear to me that the
nulling out of the ',' and updating of *opt we're doing are okay in
the error case; that's only specified for the case where a matching
key was found.

The (informative, i.e. non-normative) APPLICATION USAGE text in the
spec does mention:

    "The value of *valuep when getsubopt() returns -1 is unspecified.
    Historical implementations provide various incompatible extensions
    to allow an application to access the suboption text that was not
    found in the keylistp array."

which is not reassuring. Generally when there were historically
incompatible variants of something, musl does not offer any of them.

> This means, *val should be set to the value *opt had upon the call of
> getsubopt on failure, but this is not the case. This fixes the behavior.
> 
> This fixes a segmentation fault in the option parsing in v4l2-ctl for the
> -c parameter. (E.g. v4l2-ctl -c foo=bar will segfault without this.)

I think there's still a discussion to be had about whether the
proposed change (or even the current behavior) is okay, but v4l2-ctl
relying on this is not a good thing, and it should be patched. AFAICT
it looks like they're trying to use getsubopt for something entirely
different from its purpose, where they really just want
strchr(foo,',')...