Skip to content

Commit

Permalink
Rollup merge of rust-lang#131505 - madsmtm:darwin_user_temp_dir, r=dt…
Browse files Browse the repository at this point in the history
…olnay

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
  • Loading branch information
jieyouxu authored Nov 23, 2024
2 parents c6d3625 + f98d9dd commit f860f5b
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 7 deletions.
15 changes: 12 additions & 3 deletions library/std/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
78 changes: 74 additions & 4 deletions library/std/src/sys/pal/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
})
}
Expand Down
25 changes: 25 additions & 0 deletions library/std/src/sys/pal/unix/os/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}

0 comments on commit f860f5b

Please sign in to comment.