Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use confstr(_CS_DARWIN_USER_TEMP_DIR, ...) as a TMPDIR fallback on Darwin #100824

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1996,9 +1996,9 @@ checksum = "830d08ce1d1d941e6b30645f1a0eb5643013d835ce3779a5fc208261dbe10f55"

[[package]]
name = "libc"
version = "0.2.135"
version = "0.2.137"
source = "registry+/~https://github.com/rust-lang/crates.io-index"
checksum = "68783febc7782c6c5cb401fbda4de5a9898be1762314da0bb2c10ced61f18b0c"
checksum = "fc7fcc620a3bff7cdd7a365be3376c97191aeaccc2a603e600951e452615bf89"
dependencies = [
"rustc-std-workspace-core",
]
Expand Down
15 changes: 12 additions & 3 deletions library/std/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -600,19 +600,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
76 changes: 72 additions & 4 deletions library/std/src/sys/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -579,12 +579,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<usize>) -> io::Result<OsString> {
let mut buf: Vec<u8> = 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<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()) };
}
Comment on lines +600 to +619
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not entirely clear to me why we do all this work, it seems like just using a static [u8; PATH_MAX] would be sufficient, and in any case, we can always just fall back to /tmp if not.

But regardless, this works, and I guess there's not much harm in it, so I won't belabour the point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// `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")
}
}
})
}
Expand Down
25 changes: 25 additions & 0 deletions library/std/src/sys/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(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());
}
}