glibc .init_array usage for std::env::args is not safe
#105999
Comments
|
Arguably swapping two entries of
That would be a data race. However, just like multi-threaded programs cannot mutate the process environment or write to the global So, I think for concurrent programs we can already assume that there are no concurrent mutations, and if there are then that is a bug in the code that performs the mutation. What remains is the question whether we should adjust the args handling to be able to deal with NULL pointers in |
|
The problem here is that this behaviour of GLib exists since literally decades, and I'm sure we'll find other commandline parsers that are doing similar things if we search a bit. My first reaction also was that this feels like something to fix in GLib, but that doesn't seem to be a practical resolution because of the above. Skipping over |
|
I can't judge how common it is to add NULLs there, and don't have an opinion on whether we should support that -- let's see what @rust-lang/libs says. If you know of another library doing this that could be useful, though GLib doing it for decades on its own already means this is quite wide-spread. I hope GLib does this before any threads are spawned, because otherwise I'm afraid that is just inherently doomed. |
I'll search a bit later.
An application could surely do that, but that's just asking for problems and I wouldn't really worry about that. The following would work around this FWIW. Maybe should be conditional on glibc. diff --git a/library/std/src/sys/unix/args.rs b/library/std/src/sys/unix/args.rs
index a342f0f5e85..4231f0b4070 100644
--- a/library/std/src/sys/unix/args.rs
+++ b/library/std/src/sys/unix/args.rs
@@ -142,9 +142,16 @@ fn clone() -> Vec<OsString> {
let argv = ARGV.load(Ordering::Relaxed);
let argc = if argv.is_null() { 0 } else { ARGC.load(Ordering::Relaxed) };
(0..argc)
- .map(|i| {
- let cstr = CStr::from_ptr(*argv.offset(i) as *const libc::c_char);
- OsStringExt::from_vec(cstr.to_bytes().to_vec())
+ .filter_map(|i| {
+ let ptr = *argv.offset(i) as *const libc::c_char;
+ // Some C commandline parsers are replacing already handled arguments in `argv`
+ // with `NULL` so it's necessary to skip over them here.
+ if ptr.is_null() {
+ None
+ } else {
+ let cstr = CStr::from_ptr(ptr);
+ Some(OsStringExt::from_vec(cstr.to_bytes().to_vec()))
+ }
})
.collect()
} |
|
nit: s/Same/Some |
Qt does the same thing in |
|
|
|
Another approach would be parsing |
|
|
|
Skipping over NULL seems less safe; would it make sense to just stop at the first NULL? |
To the best of my knowledge, it's completely valid to parse argv by stopping at the first NULL; if an argument parser is replacing an argument with NULL, it should not expect that arguments after that point will be seen. I've seen plenty of C programs that just do |
|
For the commandline parsers from GLib and Qt that would be sufficient, yes. Considering the below, it seems OK to do that and there shouldn't really be any risk of missing useful arguments.
TIL that |
… r=ChrisDenton Stop at the first `NULL` argument when iterating `argv` Some C commandline parsers (e.g. GLib and Qt) are replacing already handled arguments in `argv` with `NULL` and move them to the end. That means that `argc` might be bigger than the actual number of non-`NULL` pointers in `argv` at this point. To handle this we simply stop iterating at the first `NULL` argument. `argv` is also guaranteed to be `NULL`-terminated so any non-`NULL` arguments after the first `NULL` can safely be ignored. Fixes rust-lang#105999
Some C commandline parsers (e.g. GLib and Qt) are replacing already handled arguments in `argv` with `NULL` and move them to the end. That means that `argc` might be bigger than the actual number of non-`NULL` pointers in `argv` at this point. To handle this we simply stop iterating at the first `NULL` argument. `argv` is also guaranteed to be `NULL`-terminated so any non-`NULL` arguments after the first `NULL` can safely be ignored. Fixes rust-lang/rust#105999
…enton Stop at the first `NULL` argument when iterating `argv` Some C commandline parsers (e.g. GLib and Qt) are replacing already handled arguments in `argv` with `NULL` and move them to the end. That means that `argc` might be bigger than the actual number of non-`NULL` pointers in `argv` at this point. To handle this we simply stop iterating at the first `NULL` argument. `argv` is also guaranteed to be `NULL`-terminated so any non-`NULL` arguments after the first `NULL` can safely be ignored. Fixes rust-lang/rust#105999
Currently in
library/std/src/sys/unix/args.rs, Rust is hooking into the glibc.init_arrayextension for retrievingargc/argveven in case Rust does not managemain()and is e.g. just loaded into a process as acdylib.While this is great and convenient, it's unfortunately not implemented in a safe way. Various C commandline argument parsers are modifying
argvwhile parsing, so the information that gets passed into.init_arraymight not be correct anymore.Examples of such parsers are
getoptparser, noting " The default is to permute the contents of argv while scanning it so that eventually all the non-options are at the end. This allows options to be given in any order, even with programs that were not written to expect this. "GOptionContextparser, which removes all already handled arguments fromargvand only leaves others (removed ones are set toNULLand moved to the end).QCoreApplicationinQCoreApplicationPrivate::processCommandLineArguments(). It shuffles around arguments inargvand removes (by setting tonullptr) arguments it handled already.This means that at least
argccan easily be too big, and Rust would read beyond the (new) end ofargv. This caused crashes in practice because of callingstrlen()onNULLwhen creating anCStraround such an "removed" argument.Now the question is how this should be handled in Rust. I see three options here
.init_arrayextension usage and handle glibc like all other libcs.init_array. This means everybody has to pay for that even if they don't use arguments, and this is theoretically still not safe if the shared library is loaded at the same time theargvarray is modified and a partially written pointer value is readNULLpointers inargvby skipping over them. This means we would lose the exact length information of theargsiterator, and this is still theoretically not safe for the same reason as 2. See Stop at the firstNULLargument when iteratingargv#106001This code was added in 2019 by #66547 (CC @leo60228)
I'd be happy to provide a PR implementing either solution once there is some agreement how to move forward here.I've created #106001 as a proposed fix for this, which implements option 3.Meta
rustc --version --verbose:Backtrace
Also
The text was updated successfully, but these errors were encountered: