Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for optind in getopt on musl libc. #31946

Merged
merged 1 commit into from
May 10, 2019
Merged

Fix for optind in getopt on musl libc. #31946

merged 1 commit into from
May 10, 2019

Conversation

fredrikekre
Copy link
Member

@fredrikekre fredrikekre commented May 6, 2019

This fixes

@test readchomp(`$exename -E "Base.JLOptions().opt_level" -O`) == "3"
and
@test readchomp(`$exename -E "Base.JLOptions().debug_level" -g`) == "2"

i.e. the bug only shows where there are a last option with a default value.

On exit optind points to the last non-opt argument of argv, but in the case where there are only options optind does not go beyond argc, except on musl libc, where it becomes argc + 1. This causes

*argcp -= optind;
to be -1 and then we try to resize ARGS to -1 here:
jl_array_grow_end(args, argc);
and crash.

I don't know if this is an acceptable fix, but feels like it should be safe on all systems.

edit: fixes #26761

points to the last non-opt argument of argv, but
in the case where there are only options optind
does not go beyond argc, except on musl libc,
where it becomes argc + 1.
@ararslan ararslan requested review from staticfloat and Keno May 6, 2019 23:23
@fredrikekre fredrikekre changed the title Fix optind in getopt on musl libc. Fix for optind in getopt on musl libc. May 7, 2019
@stevengj
Copy link
Member

Should a bug report be filed with musl as well?

@fredrikekre
Copy link
Member Author

fredrikekre commented May 10, 2019

I don't know which one is correct though, e.g. from https://www.gnu.org/software/libc/manual/html_node/Using-Getopt.html#Using-Getopt:

optind: This variable is set by getopt to the index of the next element of the argv array to be processed. Once getopt has found all of the option arguments, you can use this variable to determine where the remaining non-option arguments begin. The initial value of this variable is 1.

which could be interpreted as optind should be argc + 1 when there are only option arguments. Also, https://github.com/JuliaLang/julia/blob/master/src/getopt.h#L1 seems to be based on the musl version, only used on MSVC so maybe that has the same problem

julia/src/Makefile

Lines 50 to 52 in 5486cc2

ifeq ($(USEMSVC), 1)
SRCS += getopt
endif

@staticfloat staticfloat merged commit 779ac77 into master May 10, 2019
@fredrikekre fredrikekre deleted the fe/getopt-musl branch May 10, 2019 16:24
@staticfloat
Copy link
Member

you can use this variable to determine where the remaining non-option arguments begin

IMO it's a bug that it points off the end of the array; I think it's worthwhile notifying the musl developers. They're probably aware and they'll probably close it as wontfix, but might as well try to be good citizens.

@stevengj
Copy link
Member

stevengj commented May 10, 2019

The POSIX getopt documentation is quite explicit about optind:

If the resulting value of optind is greater than argc, this indicates a missing option-argument, and getopt() shall return an error indication.

My reading is that on a successful call POSIX requires 1 ≤ optind ≤ argc, and if the arguments are all valid options then you should have optind == argc.

@ecsx1 ecsx1 mentioned this pull request Jul 5, 2019
32 tasks
@KristofferC KristofferC mentioned this pull request Aug 26, 2019
55 tasks
@KristofferC KristofferC added the bugfix This change fixes an existing bug label Sep 9, 2019
@richfelker
Copy link

As noted by @stevengj, POSIX is very explicit about requiring this behavior; it's not a bug in musl:

If the option was the last character in the string pointed to by an element of argv, then optarg shall contain the next element of argv, and optind shall be incremented by 2. If the resulting value of optind is greater than argc, this indicates a missing option-argument, and getopt() shall return an error indication.

If other implementations are not doing this it should probably be reported as a bug against them.

@ViralBShah ViralBShah added the compiler:musl Support for musl linked binaries on linux instead of glibc label Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:musl Support for musl linked binaries on linux instead of glibc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failures on Alpine Linux
7 participants