Fallback to .init_array when no arguments are available on glibc Linux

[leo60228]

Linux is one of the only platforms where std::env::args doesn't work in a cdylib.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[joshtriplett]

You can't always assume that /proc is mounted (for instance, in a container environment or other types of embedded Linux uses). I don't think we should parse that to get arguments. It has the danger of working in the common case and breaking unexpectedly in unusual environments.

This is a known limitation on Linux, that you can't reliably get the program arguments except by saving them in the main entry point. I don't think we should try to work around that. (And honestly, libraries should ask the application to pass in the arguments the library should use, which might not be the original arguments of the program.)

[joshtriplett]

I would personally propose closing this, unless a method arises to get the arguments out of the C library or similar.

We really appreciate your contribution, however.

[leo60228]

You can put an extern "C" fn( libc::c_int, *const *const libc::c_char, *const *const libc::c_char, ) pointer in the .init_array section and it'll get called with argc, argv, and envp. This should be more portable, but I'm not sure the best way to implement it in std.

[leo60228]

This is implemented in both glibc and bionic (glibc is the only libc that passes argc and argv), and likely other libcs.

[leo60228]

There's also this approach that assumes __environ is a pointer to the stack (which is true on all major libcs), and uses the fact that arguments are always directly before the environment on the stack. I've confirmed this is the case on x86 and x86_64, and it's probably the case on other architectures. https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=732b382e6f91c3faa6e6a3a8d2a23c99

[leo60228]

@joshtriplett What do you think about these options? Using .init_array only works on glibc, but walking the stack is both hacky and non-constant-time.

[leo60228]

I've implemented these at https://docs.rs/libargs.

[joshtriplett]

On November 20, 2019 6:02:33 AM PST, leo60228 ***@***.***> wrote: @joshtriplett What do you think about these options? Using `.init_array` only works on glibc, but walking the stack is both hacky and non-constant-time.

Walking the stack seems like a bad idea, considering that it's legal to change the stack before loading a library, or for that matter call the library on a thread other than the initial thread. And you'd have no way to know if you got valid data. `.init_array` seems like a reasonable thing for the glibc target to try, given that you'll know if it got called or not. And if someone strips out the `.init_array` section, this fails gracefully. Only do this in the glibc target, though, and we need to document it so that library builders know about it.

[leo60228]

Where should this be documented?

[joshtriplett]

On November 21, 2019 5:21:30 AM PST, leo60228 ***@***.***> wrote: Where should this be documented?

In the documentation of the args functions.

[joshtriplett]

Can you please add a platform-specific test for this behavior, so we know if we've done something that makes it stop working?

[leo60228]

In the documentation of the args functions.
In std::env or std::sys::unix::args?

[joshtriplett]

One other thought: if we initialize arguments this way, we don't also need to initialize them in our main function in binaries. Can you compile out that code on linux-gnu?

[joshtriplett]

Can you confirm that this works in statically linked libraries and statically linked binaries (with static glibc)?

[leo60228]

If the sys::args::init is disabled in rt::init, wouldn't that make a separate test unnecessary? If not, how can I write that test?

[leo60228]

Can you confirm that this works in statically linked libraries and statically linked binaries (with static glibc)?

I've been having stability issues with my system, and compiling rustc causes a freeze, so I've been using ./x.py test --stage 0 --no-doc src/libstd. Is there a way to use this libstd to compile a program?

[rust-highfive]

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-11-21T16:22:29.1607324Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-21T16:22:29.1620984Z ##[command]git config gc.auto 0
2019-11-21T16:22:29.1624148Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-21T16:22:29.1627504Z ##[command]git config --get-all http.proxy
2019-11-21T16:22:29.1631478Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66547/merge:refs/remotes/pull/66547/merge
---
2019-11-21T16:27:58.7502788Z tidy check
2019-11-21T16:27:59.7506603Z * 588 error codes
2019-11-21T16:27:59.7506725Z * highest error code: E0744
2019-11-21T16:28:00.0822242Z * 270 features
2019-11-21T16:28:00.3027299Z tidy error: /checkout/src/libstd/rt.rs:44: platform-specific cfg: cfg(not(all(target_os = "linux", target_env = "gnu")))
2019-11-21T16:28:00.9006434Z some tidy checks failed
2019-11-21T16:28:00.9006539Z Found 441 error codes
2019-11-21T16:28:00.9006648Z Found 0 error codes with no tests
2019-11-21T16:28:00.9006692Z Done!
2019-11-21T16:28:00.9006692Z Done!
2019-11-21T16:28:00.9006720Z 2019-11-21T16:28:00.9006745Z 2019-11-21T16:28:00.9007531Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-21T16:28:00.9007630Z 2019-11-21T16:28:00.9007675Z 2019-11-21T16:28:00.9014782Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-21T16:28:00.9015070Z Build completed unsuccessfully in 0:01:19
2019-11-21T16:28:00.9015070Z Build completed unsuccessfully in 0:01:19
2019-11-21T16:28:00.9063969Z == clock drift check ==
2019-11-21T16:28:00.9074767Z local time: Thu Nov 21 16:28:00 UTC 2019
2019-11-21T16:28:01.1702061Z network time: Thu, 21 Nov 2019 16:28:01 GMT
2019-11-21T16:28:01.1702207Z == end clock drift check ==
2019-11-21T16:28:02.5068956Z 2019-11-21T16:28:02.5161810Z ##[error]Bash exited with code '1'.
2019-11-21T16:28:02.5187570Z ##[section]Starting: Checkout
2019-11-21T16:28:02.5189688Z ==============================================================================
2019-11-21T16:28:02.5189756Z Task : Get sources
2019-11-21T16:28:02.5189803Z Description : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[leo60228]

2019-11-21T16:28:00.3027299Z tidy error: /checkout/src/libstd/rt.rs:44: platform-specific cfg: cfg(not(all(target_os = "linux", target_env = "gnu")))

Why is this an error?

[leo60228]

Oh, it's because cfgs need to be in sys. Should I just keep executing args::init no matter what, then? That what it'd still work even if .init_array gets removed (like you mentioned).

[joshtriplett]

@leo60228

In std::env or std::sys::unix::args?

In std::env::args and std::env::args_os where people will actually see it in the documentation.

Oh, it's because cfgs need to be in sys. Should I just keep executing args::init no matter what, then?

How about marking args::init as #[inline(always)], and making it a no-op on linux-gnu? Your .init_array code can call the "real" implementation. That way there's no cfg in the entry point code, and there's no duplicate initialization of arguments.

Also, investigating further, this does work in a binary, including a statically linked binary. Also worth noting, a glibc-based binary or library has content in .init_array by default, and will segfault in __libc_csu_init if you strip out .init_array, so please ignore my previous comments about the possibility of stripping out .init_array. Given that glibc uses this for its own purposes, using it in Rust doesn't seem unreasonable to me. It'll get left out when building for no_std.

So, at this point, I think we just need a test, and a bit of cfg logic to make rt::init's arg initialization a no-op.

[joshtriplett]

Thank you for all the work you've put into this!

@rust-lang/libs, does this seem reasonable to you? I think this seems worth doing.

[alexcrichton]

I would agree this seems reasonable to land, supporting argc/argv access in a cdylib is best effort on a per-platform basis, so adding platform specific code to support it somewhere seems fine by me.

[joshtriplett]

What's the libs process here? Should this go through rfcbot for libs, or since it doesn't add new API, is the current expressed support sufficient?

@leo60228 Could you please rebase -i into a set of logical commits (that don't include reverts or fixes to previous patches, for instance)?

[JohnCSimon]

Ping from triage
@leo60228 Could you please rebase this PR as per the change request?
Thank you.

Looks like it's been rebased.

[dtolnay]

This looks good to me, thanks @leo60228!

[dtolnay]

@bors r+

[bors]

📌 Commit c6bcea9 has been approved by dtolnay

[bors]

⌛ Testing commit c6bcea9 with merge 3907d59...

[bors]

☀️ Test successful - checks-azure
Approved by: dtolnay
Pushing 3907d59 to master...

[rust-highfive]

📣 Toolstate changed by #66547!

Tested on commit 3907d59.
Direct link to PR: #66547

💔 miri on linux: test-pass → test-fail (cc @oli-obk@eddyb@RalfJung, @rust-lang/infra).

[sdroege]

FYI this is actually not safe like this, see #105999