Rollup merge of rust-lang#131505 - madsmtm:darwin_user_temp_dir, r=dtolnay

jieyouxu · web-flow · commit 9a152e2565f0 · 2024-11-23T20:19:52.000+08:00
use `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)` as a `TMPDIR` fallback on Darwin Rebased version of rust-lang#100824, FCP has completed there. Motivation from rust-lang#100824 (comment): > This is a behavioral change in an edge case on Darwin platforms (macOS, iOS, ...). > > Specifically, this changes it so that iff `TMPDIR` is unset in the environment, then we use `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)` to query the user temporary directory (previously we just returned `"/tmp"`). If this fails (probably possible in a sandboxed program), only then do we fallback to `"/tmp"` (as before). > > The motivations here are two-fold: > > 1. This is better for security, and is in line with the [platform security recommendations](https://developer.apple.com/library/archive/documentation/Security/Conceptual/SecureCodingGuide/Articles/RaceConditions.html#//apple_ref/doc/uid/TP40002585-SW10), as it is unavailable to other users (although it is the same value as seen by all other processes run by the same user). > 2. This is a more consistent fallback for when `getenv("TMPDIR")` is unavailable, as `$TMPDIR` is usually initialized to the `DARWIN_USER_TEMP_DIR`. > > It seems quite unlikely that anybody will break because of this, and I think it falls under the carve-out we have for platform specific behavior: https://doc.rust-lang.org/nightly/std/io/index.html#platform-specific-behavior. Closes rust-lang#99608. Closes rust-lang#100824. ``@rustbot`` label O-apple T-libs-api r? Dylan-DPC
diff --git a/std/src/env.rs b/std/src/env.rs
@@ -653,19 +653,28 @@ pub fn home_dir() -> Option<PathBuf> {
 /// 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
@@ -698,12 +698,82 @@ 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
+//
+// FIXME: Support `confstr` in Miri.
+#[cfg(all(target_vendor = "apple", not(miri)))]
+fn confstr(key: c_int, size_hint: Option<usize>) -> io::Result<OsString> {
+    let mut buf: Vec<u8> = 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
+            // 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<u8>` instead of `Vec<MaybeUninit<u8>>`, 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::<c_char>(), 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(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),
+        // 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(all(target_vendor = "apple", not(miri)))] {
+                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
@@ -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(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| {
+            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());
+    }
+}
