liblibc: Fix prototype of functions taking char *const argv[]
#25641
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
geofft
changed the title
char *const argv[]
|
Edited to cover |
alexcrichton
added
the
T-libs-api
|
Thanks for the PR @geofft! I think we may want to go ahead and lump in #25642 here as well. I think it's fine to make these changes in terms of an API backwards-compatible sense because I wonder, perhaps, though if |
|
I'm fine with making getopt take That said, is there even a reason to keep it in liblibc? It's not used in-tree, and there's no stable way to get the process arguments in the form they showed up to libc's main: it's much easier to use the |
|
Unfortunately all of liblibc is exported as a stable API under the libc crate, so we can't just remove it :( |
|
It's 0.x, right? (I vaguely assumed that was on purpose.) I'd be happy with leaving getopt as is for now and removing it when you have an excuse to do a 0.2, if you want to do it that way. |
|
It is, yeah, but this doesn't seem like enough reason to bump to 0.2 just yet |
|
OK, I've squashed the two together and rebased on current HEAD, and changed getopt to unconditionally take Can we stick a |
The execv family of functions do not modify their arguments, so they do not need mutable pointers. The C prototypes take a constant array of mutable C-strings, but that's a legacy quirk from before C had const (since C string literals have type `char *`). The Rust prototypes had `*mut` in the wrong place, anyway: to match the C prototypes, it should have been `*const *mut c_char`. But it is safe to pass constant strings (like string literals) to these functions. getopt is a special case, since GNU getopt modifies its arguments despite the `const` claim in the prototype. It is apparently only well-defined to call getopt on the actual argc and argv parameters passed to main, anyway. Change it to take `*mut *mut c_char` for an attempt at safety, but probably nobody should be using it from Rust, since there's no great way to get at the parameters as passed to main. Also fix the one caller of execvp in libstd, which now no longer needs an unsafe cast. Fixes rust-lang#16290.
The `execv` family of functions and `getopt` are prototyped somewhat strangely in C: they take a `char *const argv[]` (and `envp`, for `execve`), an immutable array of mutable C strings -- in other words, a `char *const *argv` or `argv: *const *mut c_char`. The current Rust binding uses `*mut *const c_char`, which is backwards (a mutable array of constant C strings). That said, these functions do not actually modify their arguments. Once upon a time, C didn't have `const`, and to this day, string literals in C have type `char *` (`*mut c_char`). So an array of string literals has type `char * []`, equivalent to `char **` in a function parameter (Rust `*mut *mut c_char`). C allows an implicit cast from `T **` to `T * const *` (`*const *mut T`) but not to `const T * const *` (`*const *const T`). Therefore, prototyping `execv` as taking `const char * const argv[]` would have broken existing code (by requiring an explicit cast), despite being more correct. So, even though these functions don't need mutable data, they're prototyped as if they do. While it's theoretically possible that an implementation could choose to use its freedom to modify the mutable data, such an implementation would break the innumerable users of `execv`-family functions that call them with string literals. Such an implementation would also break `std::process`, which currently works around this with an unsafe `as *mut _` cast, and assumes that `execvp` secretly does not modify its argument. Furthermore, there's nothing useful to be gained by being able to write to the strings in `argv` themselves but not being able to write to the array containing those strings (you can't reorder arguments, add arguments, increase the length of any parameter, etc.). So, despite the C prototype with its legacy C problems, it's simpler for everyone for Rust to consider these functions as taking `*const *const c_char`, and it's also safe to do so. Rust does not need to expose the mistakes of C here. This patch makes that change, and drops the unsafe cast in `std::process` since it's now unnecessary.
The
execvfamily of functions andgetoptare prototyped somewhat strangely in C: they take achar *const argv[](andenvp, forexecve), an immutable array of mutable C strings -- in other words, achar *const *argvorargv: *const *mut c_char. The current Rust binding uses*mut *const c_char, which is backwards (a mutable array of constant C strings).That said, these functions do not actually modify their arguments. Once upon a time, C didn't have
const, and to this day, string literals in C have typechar *(*mut c_char). So an array of string literals has typechar * [], equivalent tochar **in a function parameter (Rust*mut *mut c_char). C allows an implicit cast fromT **toT * const *(*const *mut T) but not toconst T * const *(*const *const T). Therefore, prototypingexecvas takingconst char * const argv[]would have broken existing code (by requiring an explicit cast), despite being more correct. So, even though these functions don't need mutable data, they're prototyped as if they do.While it's theoretically possible that an implementation could choose to use its freedom to modify the mutable data, such an implementation would break the innumerable users of
execv-family functions that call them with string literals. Such an implementation would also breakstd::process, which currently works around this with an unsafeas *mut _cast, and assumes thatexecvpsecretly does not modify its argument. Furthermore, there's nothing useful to be gained by being able to write to the strings inargvthemselves but not being able to write to the array containing those strings (you can't reorder arguments, add arguments, increase the length of any parameter, etc.).So, despite the C prototype with its legacy C problems, it's simpler for everyone for Rust to consider these functions as taking
*const *const c_char, and it's also safe to do so. Rust does not need to expose the mistakes of C here. This patch makes that change, and drops the unsafe cast instd::processsince it's now unnecessary.