lib/libc: fix bug in parsing in getopt #1918

Kintaro · 2025-12-06T12:49:19Z

getopt had an issue when parsing optional arguments if they had a space between the opion and its value.

Request: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=291374

Tested with bricoler run freebsd-src-regression-suite

github-actions · 2025-12-06T12:49:28Z

Thank you for taking the time to contribute to FreeBSD!
All issues resolved.

Kintaro · 2025-12-06T12:50:20Z

This probably needs to be backported, but as this is my first contribution I am not sure about the proper workflow on this._

jlduran

It should manually pass the following three tests (at least):

$ ./getoptest -l -w -d14 -s /usr/local/admin/tftproot -u admin -U 0
ch is l optarg is  NULL
ch is w optarg is  NULL
ch is d optarg is 14
ch is s optarg is /usr/local/admin/tftproot
ch is u optarg is admin
ch is U optarg is 0

$ ./getoptest -l -w -d 14 -s /usr/local/admin/tftproot -u admin -U 0
ch is l optarg is  NULL
ch is w optarg is  NULL
ch is d optarg is 14 # XXX <- (Or is it NULL? I'll have to test on Linux)
ch is s optarg is /usr/local/admin/tftproot
ch is u optarg is admin
ch is U optarg is 0

and

$ ./getoptest -l -w -d -s /usr/local/admin/tftproot -u admin -U 0
ch is l optarg is  NULL
ch is w optarg is  NULL
ch is d optarg is  NULL
ch is s optarg is /usr/local/admin/tftproot
ch is u optarg is admin
ch is U optarg is 0

jlduran · 2025-12-06T13:37:46Z

lib/libc/stdlib/getopt.c

 		if (*place)
 			optarg = place;
-		else if (oli[2] == ':')
+        else if (oli[2] == ':') {


The style(9) fixes I was referring to in the bug report should be addressed, maybe using NetBSD style guide (https://cvsweb.netbsd.org/bsdweb.cgi/src/share/misc/style?rev=HEAD&content-type=text/x-cvsweb-markup).
Basically, use tabs, not spaces for indentation, and braces around single-line bodies are optional.

Thanks for the review. I fixed the tabs. Not sure on the braces. It is a "single instruction" but it is another nested if/else. Not contesting any of it, just unsure which one would be preferred.

As for the tests:

rootnode@x220 ~/c/c/291374> ./getopttest -l -w -d14 -s /usr/local/admin/tftproot -u admin -U 0 ch is l optarg is NULL ch is w optarg is NULL ch is d optarg is 14 ch is s optarg is /usr/local/admin/tftproot ch is u optarg is admin ch is U optarg is 0

rootnode@x220 ~/c/c/291374> ./getopttest -l -w -d 14 -s /usr/local/admin/tftproot -u admin -U 0 ch is l optarg is NULL ch is w optarg is NULL ch is d optarg is 14 ch is s optarg is /usr/local/admin/tftproot ch is u optarg is admin ch is U optarg is 0

rootnode@x220 ~/c/c/291374> ./getopttest -l -w -d -s /usr/local/admin/tftproot -u admin -U 0 ch is l optarg is NULL ch is w optarg is NULL ch is d optarg is NULL ch is s optarg is /usr/local/admin/tftproot ch is u optarg is admin ch is U optarg is 0

Unless I'm missing something, this patch does not match the one from the bug report, nor it passes the tests above.

Unless I'm missing something, this patch does not match the one from the bug report, nor it passes the tests above.

I mixed up the two branches between my 2 machines. One wasn't up to date. You refer to the missing && nargv[optind + 1][0] != '-'?

After some testing it turned out that this would for example segfault on ./getopttest -d -14

At that point it comes down to interpretation I guess. Is the - in the value argument allowed or should it be quoted to be allowed.

The version in this PR is not segfaulting and allows for - values.

$ ./getoptest -l -w -d -s /usr/local/admin/tftproot -u admin -U 0 ch is l optarg is NULL ch is w optarg is NULL ch is d optarg is -s

Should the value 14 be missing as in the example above, then it would stop parsing at that point and return an error.

As I understand it, the man page description allows for the whitespace followed by any value.

But you are correct, the 3rd case would be broken.

I'll see if I can mitigate the segfault and get the 3rd case working. Values starting with a - might have to be quoted then.

OK, so let's add that 4th test:

./getoptest -l -w -d -14 -s /usr/local/admin/tftproot -u admin -U 0 ch is l optarg is NULL ch is w optarg is NULL ch is d optarg is NULL ./getoptest: illegal option -- 1 ch is ? optarg is NULL ./getoptest: illegal option -- 4 ch is ? optarg is NULL ch is s optarg is /usr/local/admin/tftproot ch is u optarg is admin ch is U optarg is 0

EDIT: Match the expected FreeBSD output.

You mean as the expected behavior?

You mean as the expected behavior?

Yes, unless you have a different suggestion.

Suggestion makes sense. I will make the appropriate changes. Will take bit due to timezones.

jlduran · 2025-12-08T15:52:54Z

So, I have the following patch you submitted in the Buzgilla Problem Report 291374 (adjusted to match style(9)):

--- a/lib/libc/stdlib/getopt.c
+++ b/lib/libc/stdlib/getopt.c
@@ -107,13 +107,16 @@ getopt(int nargc, char * const nargv[], const char *ostr)
                   entire next argument. */
                if (*place)
                        optarg = place;
-               else if (oli[2] == ':')
+               else if (oli[2] == ':') {
                        /*
                         * GNU Extension, for optional arguments if the rest of
                         * the argument is empty, we return NULL
                         */
-                       optarg = NULL;
-               else if (nargc > ++optind)
+                       if (nargc > optind + 1 && nargv[optind + 1][0] != '-')
+                               optarg = nargv[++optind];
+                       else
+                               optarg = NULL;
+               } else if (nargc > ++optind)
                        optarg = nargv[optind];
                else {
                        /* option-argument absent */

Do you see any issues with it?

Kintaro · 2025-12-08T16:36:59Z

So, I have the following patch you submitted in the Buzgilla Problem Report 291374 (adjusted to match style(9)):

--- a/lib/libc/stdlib/getopt.c
+++ b/lib/libc/stdlib/getopt.c
@@ -107,13 +107,16 @@ getopt(int nargc, char * const nargv[], const char *ostr)
                   entire next argument. */
                if (*place)
                        optarg = place;
-               else if (oli[2] == ':')
+               else if (oli[2] == ':') {
                        /*
                         * GNU Extension, for optional arguments if the rest of
                         * the argument is empty, we return NULL
                         */
-                       optarg = NULL;
-               else if (nargc > ++optind)
+                       if (nargc > optind + 1 && nargv[optind + 1][0] != '-')
+                               optarg = nargv[++optind];
+                       else
+                               optarg = NULL;
+               } else if (nargc > ++optind)
                        optarg = nargv[optind];
                else {
                        /* option-argument absent */

Do you see any issues with it?

Yes, in it's current state it would segfault on the ./getoptest -l -w -d -14 -s /usr/local/admin/tftproot -u admin -U 0 test. I will have to add a few checks and rethink the approach.

jlduran · 2025-12-08T16:40:01Z

So, I have the following patch you submitted in the Buzgilla Problem Report 291374 (adjusted to match style(9)):

--- a/lib/libc/stdlib/getopt.c
+++ b/lib/libc/stdlib/getopt.c
@@ -107,13 +107,16 @@ getopt(int nargc, char * const nargv[], const char *ostr)
                   entire next argument. */
                if (*place)
                        optarg = place;
-               else if (oli[2] == ':')
+               else if (oli[2] == ':') {
                        /*
                         * GNU Extension, for optional arguments if the rest of
                         * the argument is empty, we return NULL
                         */
-                       optarg = NULL;
-               else if (nargc > ++optind)
+                       if (nargc > optind + 1 && nargv[optind + 1][0] != '-')
+                               optarg = nargv[++optind];
+                       else
+                               optarg = NULL;
+               } else if (nargc > ++optind)
                        optarg = nargv[optind];
                else {
                        /* option-argument absent */

Do you see any issues with it?

Yes, in it's current state it would segfault on the ./getoptest -l -w -d -14 -s /usr/local/admin/tftproot -u admin -U 0 test. I will have to add a few checks and rethink the approach.

That is strange, I don't see any segmentation faults:

$ ./getopttest -l -w -d -14 -s /usr/local/admin/tftproot -u admin -U 0
ch is l optarg is  NULL
ch is w optarg is  NULL
ch is d optarg is  NULL
getopttest: illegal option -- 1
ch is ? optarg is  NULL
getopttest: illegal option -- 4
ch is ? optarg is  NULL
ch is s optarg is /usr/local/admin/tftproot
ch is u optarg is admin
ch is U optarg is 0

Kintaro · 2025-12-08T17:30:20Z

L

So, I have the following patch you submitted in the Buzgilla Problem Report 291374 (adjusted to match style(9)):

--- a/lib/libc/stdlib/getopt.c
+++ b/lib/libc/stdlib/getopt.c
@@ -107,13 +107,16 @@ getopt(int nargc, char * const nargv[], const char *ostr)
                   entire next argument. */
                if (*place)
                        optarg = place;
-               else if (oli[2] == ':')
+               else if (oli[2] == ':') {
                        /*
                         * GNU Extension, for optional arguments if the rest of
                         * the argument is empty, we return NULL
                         */
-                       optarg = NULL;
-               else if (nargc > ++optind)
+                       if (nargc > optind + 1 && nargv[optind + 1][0] != '-')
+                               optarg = nargv[++optind];
+                       else
+                               optarg = NULL;
+               } else if (nargc > ++optind)
                        optarg = nargv[optind];
                else {
                        /* option-argument absent */

Do you see any issues with it?

Yes, in it's current state it would segfault on the ./getoptest -l -w -d -14 -s /usr/local/admin/tftproot -u admin -U 0 test. I will have to add a few checks and rethink the approach.

That is strange, I don't see any segmentation faults:

$ ./getopttest -l -w -d -14 -s /usr/local/admin/tftproot -u admin -U 0
ch is l optarg is  NULL
ch is w optarg is  NULL
ch is d optarg is  NULL
getopttest: illegal option -- 1
ch is ? optarg is  NULL
getopttest: illegal option -- 4
ch is ? optarg is  NULL
ch is s optarg is /usr/local/admin/tftproot
ch is u optarg is admin
ch is U optarg is 0

I did a clean build. Yes, that seems to work. I will update the PR.

jlduran · 2025-12-08T18:19:28Z

Please commit the patch attached in:

#1918 (comment)

And we'll start from there.

jlduran

Thank you!

The correct trailer should be:

PR:<tab><tab>291374

getopt had an issue when parsing optional arguments if they had a space between the opion and its value. Signed-off-by: Simon Wollwage <[email protected]> PR: 291374

Kintaro · 2025-12-09T16:51:06Z

Thank you!

The correct trailer should be:
PR:<tab><tab>291374

Updated the commit message. Should hopefully be good to go now.

Kintaro · 2025-12-10T13:40:18Z

There is currently some discussion about the wording of the manpage and it's interpretation on the NetBSD mailiing list. See https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=59828

jlduran · 2025-12-11T00:14:14Z

There is currently some discussion about the wording of the manpage and it's interpretation on the NetBSD mailiing list. See https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=59828

Thank you @Kintaro for opening a bug report "upstream" (NetBSD)!

My personal opinion is that this patch is correct, and this should be the right behavior. However, if "upstream" decides that a documentation fix is a better fix (similar to the wording found on OpenBSD's manual page), then I will rescind this change, and will kindly ask you to do the same for this pull request.

Thanks again!

Kintaro · 2025-12-11T00:40:18Z

Sure, that sounds reasonable. If upstream decides to only adjust the manpage, then I can update this PR to change it to a documentation change.

I think UX wise the whitespace should be allowed, but if upstream and OpenBSD don't allow it, I can see the consistency in that regard too.

jlduran · 2025-12-11T00:41:33Z

I forgot to mention, that I base my opinion in the fact that it is indeed a GNU extension, as seen from the original commit:

NetBSD/src@78d38aa

If that is decided, then it should behave like GNU getopt(3), and the test for -d 14 should return ch is d optarg is NULL, and continue:

$ ./getopttest -l -w -d 14 -s /usr/local/admin/tftproot -u admin -U 0
ch is l optarg is  NULL
ch is w optarg is  NULL
ch is d optarg is  NULL # XXX <- NULL, not 14
ch is s optarg is /usr/local/admin/tftproot
ch is u optarg is admin
ch is U optarg is 0

Two colons mean an option takes an optional arg; if there is text in the current argv-element (i.e., in the same word as the option name itself, for example, "-oarg"), then it is returned in optarg, otherwise optarg is set to zero. This is a GNU extension.

Kintaro force-pushed the getopt branch from f8bc751 to 911e3aa Compare December 6, 2025 12:54

jlduran suggested changes Dec 6, 2025

View reviewed changes

Kintaro force-pushed the getopt branch from 911e3aa to f16c6a1 Compare December 6, 2025 14:29

jlduran added the changes-required Cannot land as is, change requested of submitter label Dec 8, 2025

Kintaro force-pushed the getopt branch from f16c6a1 to d561034 Compare December 8, 2025 17:31

Kintaro requested review from bapt and bsdimp as code owners December 8, 2025 17:31

Kintaro force-pushed the getopt branch 2 times, most recently from 454445f to 16a4ff8 Compare December 8, 2025 17:44

Kintaro force-pushed the getopt branch from 16a4ff8 to 1c9895b Compare December 9, 2025 00:23

jlduran removed the changes-required Cannot land as is, change requested of submitter label Dec 9, 2025

jlduran approved these changes Dec 9, 2025

View reviewed changes

jlduran added the ready Seems ready to go, subject to final sanity check label Dec 9, 2025

lib/libc: fix bug in parsing in getopt

35025a9

getopt had an issue when parsing optional arguments if they had a space between the opion and its value. Signed-off-by: Simon Wollwage <[email protected]> PR: 291374

Kintaro force-pushed the getopt branch from 1c9895b to 35025a9 Compare December 9, 2025 03:23

lib/libc: fix bug in parsing in getopt #1918

Are you sure you want to change the base?

lib/libc: fix bug in parsing in getopt #1918

Conversation

Kintaro commented Dec 6, 2025

Uh oh!

github-actions bot commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kintaro commented Dec 6, 2025

Uh oh!

jlduran left a comment

Choose a reason for hiding this comment

Uh oh!

jlduran Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

Kintaro Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

jlduran Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Kintaro Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlduran Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kintaro Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

jlduran Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Kintaro Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

jlduran commented Dec 8, 2025

Uh oh!

Kintaro commented Dec 8, 2025

Uh oh!

jlduran commented Dec 8, 2025

Uh oh!

Kintaro commented Dec 8, 2025

Uh oh!

jlduran commented Dec 8, 2025

Uh oh!

jlduran left a comment

Choose a reason for hiding this comment

Uh oh!

Kintaro commented Dec 9, 2025

Uh oh!

Kintaro commented Dec 10, 2025

Uh oh!

jlduran commented Dec 11, 2025

Uh oh!

Kintaro commented Dec 11, 2025

Uh oh!

jlduran commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Dec 6, 2025 •

edited

Loading

Kintaro Dec 8, 2025 •

edited

Loading

jlduran Dec 8, 2025 •

edited

Loading