From bc6398107f82c809c11eabd5bef655959c773341 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Thu, 18 Aug 2022 20:23:01 -0700 Subject: [PATCH 1/4] use `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)` as a `TMPDIR` fallback on darwin --- std/src/env.rs | 15 +++++-- std/src/sys/pal/unix/os.rs | 76 ++++++++++++++++++++++++++++++-- std/src/sys/pal/unix/os/tests.rs | 25 +++++++++++ 3 files changed, 109 insertions(+), 7 deletions(-) diff --git a/std/src/env.rs b/std/src/env.rs index 97a1b846a91a4..5a2deada6c355 100644 --- a/std/src/env.rs +++ b/std/src/env.rs @@ -653,19 +653,28 @@ pub fn home_dir() -> Option { /// may result in "insecure temporary file" security vulnerabilities. Consider /// using a crate that securely creates temporary files or directories. /// +/// Note that the returned value may be a symbolic link, not a directory. +/// /// # Platform-specific behavior /// /// On Unix, returns the value of the `TMPDIR` environment variable if it is -/// set, otherwise for non-Android it returns `/tmp`. On Android, since there -/// is no global temporary folder (it is usually allocated per-app), it returns -/// `/data/local/tmp`. +/// set, otherwise the value is OS-specific: +/// - On Android, there is no global temporary folder (it is usually allocated +/// per-app), it returns `/data/local/tmp`. +/// - On Darwin-based OSes (macOS, iOS, etc) it returns the directory provided +/// by `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)`, as recommended by [Apple's +/// security guidelines][appledoc]. +/// - On all other unix-based OSes, it returns `/tmp`. +/// /// On Windows, the behavior is equivalent to that of [`GetTempPath2`][GetTempPath2] / /// [`GetTempPath`][GetTempPath], which this function uses internally. +/// /// Note that, this [may change in the future][changes]. /// /// [changes]: io#platform-specific-behavior /// [GetTempPath2]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppath2a /// [GetTempPath]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppatha +/// [appledoc]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/SecureCodingGuide/Articles/RaceConditions.html#//apple_ref/doc/uid/TP40002585-SW10 /// /// ```no_run /// use std::env; diff --git a/std/src/sys/pal/unix/os.rs b/std/src/sys/pal/unix/os.rs index f983d174ed61c..7db037d02eba0 100644 --- a/std/src/sys/pal/unix/os.rs +++ b/std/src/sys/pal/unix/os.rs @@ -698,12 +698,80 @@ pub fn page_size() -> usize { unsafe { libc::sysconf(libc::_SC_PAGESIZE) as usize } } +// Returns the value for [`confstr(key, ...)`][posix_confstr]. Currently only +// used on Darwin, but should work on any unix (in case we need to get +// `_CS_PATH` or `_CS_V[67]_ENV` in the future). +// +// [posix_confstr]: +// https://pubs.opengroup.org/onlinepubs/9699919799/functions/confstr.html +#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos"))] +fn confstr(key: c_int, size_hint: Option) -> io::Result { + let mut buf: Vec = Vec::new(); + let mut bytes_needed_including_nul = size_hint + .unwrap_or_else(|| { + // Treat "None" as "do an extra call to get the length". In theory + // we could move this into the loop below, but it's hard to do given + // that it isn't 100% clear if it's legal to pass 0 for `len` when + // the buffer isn't null. + unsafe { libc::confstr(key, core::ptr::null_mut(), 0) } + }) + .max(1); + // If the value returned by `confstr` is greater than the len passed into + // it, then the value was truncated, meaning we need to retry. Note that + // while `confstr` results don't seem to change for a process, it's unclear + // if this is guaranteed anywhere, so looping does seem required. + while bytes_needed_including_nul > buf.capacity() { + // We write into the spare capacity of `buf`. This lets us avoid + // changing buf's `len`, which both simplifies `reserve` computation, + // allows working with `Vec` instead of `Vec>`, and + // may avoid a copy, since the Vec knows that none of the bytes are needed + // when reallocating (well, in theory anyway). + buf.reserve(bytes_needed_including_nul); + // `confstr` returns + // - 0 in the case of errors: we break and return an error. + // - The number of bytes written, iff the provided buffer is enough to + // hold the entire value: we break and return the data in `buf`. + // - Otherwise, the number of bytes needed (including nul): we go + // through the loop again. + bytes_needed_including_nul = + unsafe { libc::confstr(key, buf.as_mut_ptr().cast::(), buf.capacity()) }; + } + // `confstr` returns 0 in the case of an error. + if bytes_needed_including_nul == 0 { + return Err(io::Error::last_os_error()); + } + // Safety: `confstr(..., buf.as_mut_ptr(), buf.capacity())` returned a + // non-zero value, meaning `bytes_needed_including_nul` bytes were + // initialized. + unsafe { + buf.set_len(bytes_needed_including_nul); + // Remove the NUL-terminator. + let last_byte = buf.pop(); + // ... and smoke-check that it *was* a NUL-terminator. + assert_eq!(last_byte, Some(0), "`confstr` provided a string which wasn't nul-terminated"); + }; + Ok(OsString::from_vec(buf)) +} + +#[cfg(target_vendor = "apple")] +fn darwin_temp_dir() -> PathBuf { + confstr(libc::_CS_DARWIN_USER_TEMP_DIR, Some(64)).map(PathBuf::from).unwrap_or_else(|_| { + // It failed for whatever reason (there are several possible reasons), + // so return the global one. + PathBuf::from("/tmp") + }) +} + pub fn temp_dir() -> PathBuf { crate::env::var_os("TMPDIR").map(PathBuf::from).unwrap_or_else(|| { - if cfg!(target_os = "android") { - PathBuf::from("/data/local/tmp") - } else { - PathBuf::from("/tmp") + cfg_if::cfg_if! { + if #[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos"))] { + darwin_temp_dir() + } else if #[cfg(target_os = "android")] { + PathBuf::from("/data/local/tmp") + } else { + PathBuf::from("/tmp") + } } }) } diff --git a/std/src/sys/pal/unix/os/tests.rs b/std/src/sys/pal/unix/os/tests.rs index efc29955b05fe..ab9742fc677e9 100644 --- a/std/src/sys/pal/unix/os/tests.rs +++ b/std/src/sys/pal/unix/os/tests.rs @@ -21,3 +21,28 @@ fn test_parse_glibc_version() { assert_eq!(parsed, super::parse_glibc_version(version_str)); } } + +// Smoke check `confstr`, do it for several hint values, to ensure our resizing +// logic is correct. +#[test] +#[cfg(target_os = "macos")] +fn test_confstr() { + for key in [libc::_CS_DARWIN_USER_TEMP_DIR, libc::_CS_PATH] { + let value_nohint = super::confstr(key, None).unwrap_or_else(|e| { + panic!("confstr({key}, None) failed: {e:?}"); + }); + let end = (value_nohint.len() + 1) * 2; + for hint in 0..end { + assert_eq!( + super::confstr(key, Some(hint)).as_deref().ok(), + Some(&*value_nohint), + "confstr({key}, Some({hint})) failed", + ); + } + } + // Smoke check that we don't loop forever or something if the input was not valid. + for hint in [None, Some(0), Some(1)] { + let hopefully_invalid = 123456789_i32; + assert!(super::confstr(hopefully_invalid, hint).is_err()); + } +} From 5b16abea8d556dcbe204617cbe15314687251f37 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 10 Oct 2024 18:41:55 +0200 Subject: [PATCH 2/4] Prefer `target_vendor = "apple"` on confstr --- std/src/sys/pal/unix/os.rs | 4 ++-- std/src/sys/pal/unix/os/tests.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/std/src/sys/pal/unix/os.rs b/std/src/sys/pal/unix/os.rs index 7db037d02eba0..9750c32eed5e5 100644 --- a/std/src/sys/pal/unix/os.rs +++ b/std/src/sys/pal/unix/os.rs @@ -704,7 +704,7 @@ pub fn page_size() -> usize { // // [posix_confstr]: // https://pubs.opengroup.org/onlinepubs/9699919799/functions/confstr.html -#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos"))] +#[cfg(target_vendor = "apple")] fn confstr(key: c_int, size_hint: Option) -> io::Result { let mut buf: Vec = Vec::new(); let mut bytes_needed_including_nul = size_hint @@ -765,7 +765,7 @@ fn darwin_temp_dir() -> PathBuf { pub fn temp_dir() -> PathBuf { crate::env::var_os("TMPDIR").map(PathBuf::from).unwrap_or_else(|| { cfg_if::cfg_if! { - if #[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos"))] { + if #[cfg(target_vendor = "apple")] { darwin_temp_dir() } else if #[cfg(target_os = "android")] { PathBuf::from("/data/local/tmp") diff --git a/std/src/sys/pal/unix/os/tests.rs b/std/src/sys/pal/unix/os/tests.rs index ab9742fc677e9..a84086037ce0b 100644 --- a/std/src/sys/pal/unix/os/tests.rs +++ b/std/src/sys/pal/unix/os/tests.rs @@ -25,7 +25,7 @@ fn test_parse_glibc_version() { // Smoke check `confstr`, do it for several hint values, to ensure our resizing // logic is correct. #[test] -#[cfg(target_os = "macos")] +#[cfg(target_vendor = "apple")] fn test_confstr() { for key in [libc::_CS_DARWIN_USER_TEMP_DIR, libc::_CS_PATH] { let value_nohint = super::confstr(key, None).unwrap_or_else(|e| { From 14aef3d76cf0a31c913e70cf4675422ce0d11ee1 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 10 Oct 2024 18:57:44 +0200 Subject: [PATCH 3/4] Use with_capacity(0) because we're reading the capacity later on --- std/src/sys/pal/unix/os.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/std/src/sys/pal/unix/os.rs b/std/src/sys/pal/unix/os.rs index 9750c32eed5e5..d26a1bdef74b4 100644 --- a/std/src/sys/pal/unix/os.rs +++ b/std/src/sys/pal/unix/os.rs @@ -706,7 +706,7 @@ pub fn page_size() -> usize { // https://pubs.opengroup.org/onlinepubs/9699919799/functions/confstr.html #[cfg(target_vendor = "apple")] fn confstr(key: c_int, size_hint: Option) -> io::Result { - let mut buf: Vec = Vec::new(); + let mut buf: Vec = Vec::with_capacity(0); let mut bytes_needed_including_nul = size_hint .unwrap_or_else(|| { // Treat "None" as "do an extra call to get the length". In theory From 6ce7e79948ee4bade00ce0f0a7810295c0a7f4d9 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 10 Oct 2024 20:45:39 +0200 Subject: [PATCH 4/4] Don't try to use confstr in Miri --- std/src/sys/pal/unix/os.rs | 8 +++++--- std/src/sys/pal/unix/os/tests.rs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/std/src/sys/pal/unix/os.rs b/std/src/sys/pal/unix/os.rs index d26a1bdef74b4..f207131ddf332 100644 --- a/std/src/sys/pal/unix/os.rs +++ b/std/src/sys/pal/unix/os.rs @@ -704,7 +704,9 @@ pub fn page_size() -> usize { // // [posix_confstr]: // https://pubs.opengroup.org/onlinepubs/9699919799/functions/confstr.html -#[cfg(target_vendor = "apple")] +// +// FIXME: Support `confstr` in Miri. +#[cfg(all(target_vendor = "apple", not(miri)))] fn confstr(key: c_int, size_hint: Option) -> io::Result { let mut buf: Vec = Vec::with_capacity(0); let mut bytes_needed_including_nul = size_hint @@ -753,7 +755,7 @@ fn confstr(key: c_int, size_hint: Option) -> io::Result { Ok(OsString::from_vec(buf)) } -#[cfg(target_vendor = "apple")] +#[cfg(all(target_vendor = "apple", not(miri)))] fn darwin_temp_dir() -> PathBuf { confstr(libc::_CS_DARWIN_USER_TEMP_DIR, Some(64)).map(PathBuf::from).unwrap_or_else(|_| { // It failed for whatever reason (there are several possible reasons), @@ -765,7 +767,7 @@ fn darwin_temp_dir() -> PathBuf { pub fn temp_dir() -> PathBuf { crate::env::var_os("TMPDIR").map(PathBuf::from).unwrap_or_else(|| { cfg_if::cfg_if! { - if #[cfg(target_vendor = "apple")] { + if #[cfg(all(target_vendor = "apple", not(miri)))] { darwin_temp_dir() } else if #[cfg(target_os = "android")] { PathBuf::from("/data/local/tmp") diff --git a/std/src/sys/pal/unix/os/tests.rs b/std/src/sys/pal/unix/os/tests.rs index a84086037ce0b..63a1cc1e94a1d 100644 --- a/std/src/sys/pal/unix/os/tests.rs +++ b/std/src/sys/pal/unix/os/tests.rs @@ -25,7 +25,7 @@ fn test_parse_glibc_version() { // Smoke check `confstr`, do it for several hint values, to ensure our resizing // logic is correct. #[test] -#[cfg(target_vendor = "apple")] +#[cfg(all(target_vendor = "apple", not(miri)))] fn test_confstr() { for key in [libc::_CS_DARWIN_USER_TEMP_DIR, libc::_CS_PATH] { let value_nohint = super::confstr(key, None).unwrap_or_else(|e| {