On Sun, Mar 15, 2020 at 07:32:52PM +0100, Martijn van Duren wrote:
> tl;dr new diff below
Check with:
OPTS=r- ./getopt-test-MvD2 -r-
getopt-test-MvD2: unknown option -- -
<r> #U- |
With the unpatched code, my patch, and the original 44BSD, it's:
OPTS=r- ./getopt-test-O -r-
{./getopt-test-O} <r> <-> |
See proposed fix at the end.
> optreset and flags. Note that on line 317 we increment the pointer on
> options. This means that the "oli == options" check in your code is
> not needed and even introduces a bug when someone would be stupid
> enough to create an optstring which starts with "--", where a "-"
> option stop to being recognized:
Yes, that check was dumb, it should be removed.
> On 3/15/20 11:21 AM, 0xef967...@gmail.com wrote:
> > Notice that OpenBSD's getopt(3) explicitly allows to use "-" as an
> > option character, and there may be a /separate/ bug related to that,
> > but my patch doesn't change or affect it in any way.
>
> This is in line with getopt(3) and getopt_long(3) expands on that
> saying it's (at least) for su(1). So if we want to remove this
> (mis)feature a lot more scrutiny needs to be done and I'm not feeling
> like going down that rab(bit|ies)-hole any time soon.
I didn't say that that feature should be removed, but that there may
be another bug/incompat related to that. Since it was there since 44BSD,
it should be left as is (IMVHO).
> I did some pointer dumping inside your test-application and inside
> getopt_internal and found that the OPTS environment variable is 1 byte
> from the last place pointer. When you overwrite every byte of the
> original avs with ':' (including the terminating NUL) you expand the
> string length of the place variable inside getopt_internal and it starts
> to parse the "garbage" you see in your example.
That was of course intentional. It's perfectly legit to overwrite
the args strings (including their terminating \0). And it makes for
more dramatic testcases ;-)
> Since our manpage explicitly states that you need to set optreset I
> don't know if it's worth adding a "fix" for, if such a thing is at
No need to add another fix for it; that return (-1) that my (and your
current patch) eliminates is the only path through which -1 can be
returned /without/ setting "place = EMSG".
> if ((optchar = (int)*place++) == (int)':' ||
> - (optchar == (int)'-' && *place != '\0') ||
> - (oli = strchr(options, optchar)) == NULL) {
> - /*
> - * If the user specified "-" and '-' isn't listed in
> - * options, return -1 (non-option) as per POSIX.
> - * Otherwise, it is an unknown option character (or ':').
> - */
> - if (optchar == (int)'-' && *place == '\0')
> - return (-1);
> + (oli = strchr(options, optchar)) == NULL ||
> + (optchar == '-' &&
> + (oli == NULL || strcmp(nargv[optind], "-") != 0))) {
> if (!*place)
> ++optind;
> if (PRINT_ERROR)
... || (foo = bar) == NULL || (baz && (foo == NULL || quux))
is the same as
... || (foo = bar) == NULL || (baz && quux)
since the "||" is shortcutting.
But I still don't see how "quux" (strcmp(nargv[optind], "-") != 0)
may happen with a args which is just "-" (since it's already caught by
the "If we have "-" do nothing" check above it).
It will however BREAK a case like that mentioned at the beginning
of this message.
So make it just ".. || (foo = bar) == NULL":
- (optchar == (int)'-' && *place != '\0') ||
- (oli = strchr(options, optchar)) == NULL) {
- /*
- * If the user specified "-" and '-' isn't listed in
- * options, return -1 (non-option) as per POSIX.
- * Otherwise, it is an unknown option character (or ':').
- */
- if (optchar == (int)'-' && *place == '\0')
- return (-1);
+ (oli = strchr(options, optchar)) == NULL)) {
