std: allocate less memory in current_exe for OpenBSD#158183
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
it seems a bit over engineered to me : |
|
Yes, but this isn't about an array of characters but of pointers, and |
| @@ -234,11 +234,17 @@ pub fn current_exe() -> io::Result<PathBuf> { | |||
| unsafe { | |||
| let mut mib = [libc::CTL_KERN, libc::KERN_PROC_ARGS, libc::getpid(), libc::KERN_PROC_ARGV]; | |||
There was a problem hiding this comment.
Studying https://man.openbsd.org/sysctl.2#KERN_PROC_ARGS
...So uh is there a reason, since we have to make two sysctl calls, that we don't just ask for KERN_PROC_NARGV the first time?
...though "KERN_PROC_NARGV and KERN_PROC_NENV return the number of elements as an int in the argv or env array." has got to be the most curious phrasing possible for that.
| let mut argv = Vec::<*const libc::c_char>::with_capacity(argv_len as usize); | ||
| // ... allocate a buffer for it ... | ||
| let mut argv = | ||
| Vec::<*const libc::c_char>::with_capacity(argv_len / size_of::<*const libc::c_char>()); |
There was a problem hiding this comment.
Should we checked_div_exact and panic here just in case? I guess it seems a bit unlikely that this goes wrong (and would be a platform/libc bug anyway).
|
r=me if you're happy |
There was a problem hiding this comment.
I think the original intent of the C code was to checking if argv0 was starting with "./"
/* get realpath if possible */
if ((argv[0] != NULL) && ((*argv[0] == '.') || (*argv[0] == '/')
|| (strstr(argv[0], "/") != NULL)))There was a problem hiding this comment.
but yeah, it doesn't seems right. checking if argv0 contains "/" should be enough
495d78c to
db9b6a1
Compare
There was a problem hiding this comment.
@rustbot label +I-libs-api-nominated
This changes the behavior of std::env::current_exe on OpenBSD to not attempt to canonicalize an argv[0] if it starts with .. Previously we would canonicalize both if it starts with . or if it contains / anywhere in the path, now only the latter is checked.
IMO this is eminently reasonable and we should make the change, but in theory it seems plausible that some program is broken by this. (I suspect it's more likely that some programs are fixed by it, but hard to say).
| } | ||
| let argv0 = CStr::from_ptr(argv[0]).to_bytes(); | ||
| if argv0[0] == b'.' || argv0.iter().any(|b| *b == b'/') { | ||
| if argv0.iter().any(|b| *b == b'/') { |
There was a problem hiding this comment.
I guess technically we should FCP this breakage, let me nominate for libs-api to decide.
There was a problem hiding this comment.
We discussed this in the @rust-lang/libs-api meeting and consider this a bug fix, not something that needs an FCP. There's no reason to check for a leading ., this doesn't determine whether a string represents a relative or absolute path.
|
@bors r+ rollup |
…-Simulacrum std: allocate less memory in `current_exe` for OpenBSD This bug was introduced back in 2f42ac4 when Alex ported the `current_exe` implementation from C to Rust. `Vec::with_capacity` measures capacity in the number of elements, but `sysctl` measures it in bytes, so we need to do some conversions. CC @semarie
…-Simulacrum std: allocate less memory in `current_exe` for OpenBSD This bug was introduced back in 2f42ac4 when Alex ported the `current_exe` implementation from C to Rust. `Vec::with_capacity` measures capacity in the number of elements, but `sysctl` measures it in bytes, so we need to do some conversions. CC @semarie
…-Simulacrum std: allocate less memory in `current_exe` for OpenBSD This bug was introduced back in 2f42ac4 when Alex ported the `current_exe` implementation from C to Rust. `Vec::with_capacity` measures capacity in the number of elements, but `sysctl` measures it in bytes, so we need to do some conversions. CC @semarie
This bug was introduced back in 2f42ac4 when Alex ported the
current_exeimplementation from C to Rust.Vec::with_capacitymeasures capacity in the number of elements, butsysctlmeasures it in bytes, so we need to do some conversions.CC @semarie