Skip to content

Commit 1c3cbb4

Browse files
authored
Unrolled build for rust-lang#131505
Rollup merge of rust-lang#131505 - madsmtm:darwin_user_temp_dir, r=dtolnay 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
2 parents ff1737b + f98d9dd commit 1c3cbb4

File tree

3 files changed

+111
-7
lines changed

3 files changed

+111
-7
lines changed

library/std/src/env.rs

+12-3
Original file line numberDiff line numberDiff line change
@@ -653,19 +653,28 @@ pub fn home_dir() -> Option<PathBuf> {
653653
/// may result in "insecure temporary file" security vulnerabilities. Consider
654654
/// using a crate that securely creates temporary files or directories.
655655
///
656+
/// Note that the returned value may be a symbolic link, not a directory.
657+
///
656658
/// # Platform-specific behavior
657659
///
658660
/// On Unix, returns the value of the `TMPDIR` environment variable if it is
659-
/// set, otherwise for non-Android it returns `/tmp`. On Android, since there
660-
/// is no global temporary folder (it is usually allocated per-app), it returns
661-
/// `/data/local/tmp`.
661+
/// set, otherwise the value is OS-specific:
662+
/// - On Android, there is no global temporary folder (it is usually allocated
663+
/// per-app), it returns `/data/local/tmp`.
664+
/// - On Darwin-based OSes (macOS, iOS, etc) it returns the directory provided
665+
/// by `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)`, as recommended by [Apple's
666+
/// security guidelines][appledoc].
667+
/// - On all other unix-based OSes, it returns `/tmp`.
668+
///
662669
/// On Windows, the behavior is equivalent to that of [`GetTempPath2`][GetTempPath2] /
663670
/// [`GetTempPath`][GetTempPath], which this function uses internally.
671+
///
664672
/// Note that, this [may change in the future][changes].
665673
///
666674
/// [changes]: io#platform-specific-behavior
667675
/// [GetTempPath2]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppath2a
668676
/// [GetTempPath]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppatha
677+
/// [appledoc]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/SecureCodingGuide/Articles/RaceConditions.html#//apple_ref/doc/uid/TP40002585-SW10
669678
///
670679
/// ```no_run
671680
/// use std::env;

library/std/src/sys/pal/unix/os.rs

+74-4
Original file line numberDiff line numberDiff line change
@@ -698,12 +698,82 @@ pub fn page_size() -> usize {
698698
unsafe { libc::sysconf(libc::_SC_PAGESIZE) as usize }
699699
}
700700

701+
// Returns the value for [`confstr(key, ...)`][posix_confstr]. Currently only
702+
// used on Darwin, but should work on any unix (in case we need to get
703+
// `_CS_PATH` or `_CS_V[67]_ENV` in the future).
704+
//
705+
// [posix_confstr]:
706+
// https://pubs.opengroup.org/onlinepubs/9699919799/functions/confstr.html
707+
//
708+
// FIXME: Support `confstr` in Miri.
709+
#[cfg(all(target_vendor = "apple", not(miri)))]
710+
fn confstr(key: c_int, size_hint: Option<usize>) -> io::Result<OsString> {
711+
let mut buf: Vec<u8> = Vec::with_capacity(0);
712+
let mut bytes_needed_including_nul = size_hint
713+
.unwrap_or_else(|| {
714+
// Treat "None" as "do an extra call to get the length". In theory
715+
// we could move this into the loop below, but it's hard to do given
716+
// that it isn't 100% clear if it's legal to pass 0 for `len` when
717+
// the buffer isn't null.
718+
unsafe { libc::confstr(key, core::ptr::null_mut(), 0) }
719+
})
720+
.max(1);
721+
// If the value returned by `confstr` is greater than the len passed into
722+
// it, then the value was truncated, meaning we need to retry. Note that
723+
// while `confstr` results don't seem to change for a process, it's unclear
724+
// if this is guaranteed anywhere, so looping does seem required.
725+
while bytes_needed_including_nul > buf.capacity() {
726+
// We write into the spare capacity of `buf`. This lets us avoid
727+
// changing buf's `len`, which both simplifies `reserve` computation,
728+
// allows working with `Vec<u8>` instead of `Vec<MaybeUninit<u8>>`, and
729+
// may avoid a copy, since the Vec knows that none of the bytes are needed
730+
// when reallocating (well, in theory anyway).
731+
buf.reserve(bytes_needed_including_nul);
732+
// `confstr` returns
733+
// - 0 in the case of errors: we break and return an error.
734+
// - The number of bytes written, iff the provided buffer is enough to
735+
// hold the entire value: we break and return the data in `buf`.
736+
// - Otherwise, the number of bytes needed (including nul): we go
737+
// through the loop again.
738+
bytes_needed_including_nul =
739+
unsafe { libc::confstr(key, buf.as_mut_ptr().cast::<c_char>(), buf.capacity()) };
740+
}
741+
// `confstr` returns 0 in the case of an error.
742+
if bytes_needed_including_nul == 0 {
743+
return Err(io::Error::last_os_error());
744+
}
745+
// Safety: `confstr(..., buf.as_mut_ptr(), buf.capacity())` returned a
746+
// non-zero value, meaning `bytes_needed_including_nul` bytes were
747+
// initialized.
748+
unsafe {
749+
buf.set_len(bytes_needed_including_nul);
750+
// Remove the NUL-terminator.
751+
let last_byte = buf.pop();
752+
// ... and smoke-check that it *was* a NUL-terminator.
753+
assert_eq!(last_byte, Some(0), "`confstr` provided a string which wasn't nul-terminated");
754+
};
755+
Ok(OsString::from_vec(buf))
756+
}
757+
758+
#[cfg(all(target_vendor = "apple", not(miri)))]
759+
fn darwin_temp_dir() -> PathBuf {
760+
confstr(libc::_CS_DARWIN_USER_TEMP_DIR, Some(64)).map(PathBuf::from).unwrap_or_else(|_| {
761+
// It failed for whatever reason (there are several possible reasons),
762+
// so return the global one.
763+
PathBuf::from("/tmp")
764+
})
765+
}
766+
701767
pub fn temp_dir() -> PathBuf {
702768
crate::env::var_os("TMPDIR").map(PathBuf::from).unwrap_or_else(|| {
703-
if cfg!(target_os = "android") {
704-
PathBuf::from("/data/local/tmp")
705-
} else {
706-
PathBuf::from("/tmp")
769+
cfg_if::cfg_if! {
770+
if #[cfg(all(target_vendor = "apple", not(miri)))] {
771+
darwin_temp_dir()
772+
} else if #[cfg(target_os = "android")] {
773+
PathBuf::from("/data/local/tmp")
774+
} else {
775+
PathBuf::from("/tmp")
776+
}
707777
}
708778
})
709779
}

library/std/src/sys/pal/unix/os/tests.rs

+25
Original file line numberDiff line numberDiff line change
@@ -21,3 +21,28 @@ fn test_parse_glibc_version() {
2121
assert_eq!(parsed, super::parse_glibc_version(version_str));
2222
}
2323
}
24+
25+
// Smoke check `confstr`, do it for several hint values, to ensure our resizing
26+
// logic is correct.
27+
#[test]
28+
#[cfg(all(target_vendor = "apple", not(miri)))]
29+
fn test_confstr() {
30+
for key in [libc::_CS_DARWIN_USER_TEMP_DIR, libc::_CS_PATH] {
31+
let value_nohint = super::confstr(key, None).unwrap_or_else(|e| {
32+
panic!("confstr({key}, None) failed: {e:?}");
33+
});
34+
let end = (value_nohint.len() + 1) * 2;
35+
for hint in 0..end {
36+
assert_eq!(
37+
super::confstr(key, Some(hint)).as_deref().ok(),
38+
Some(&*value_nohint),
39+
"confstr({key}, Some({hint})) failed",
40+
);
41+
}
42+
}
43+
// Smoke check that we don't loop forever or something if the input was not valid.
44+
for hint in [None, Some(0), Some(1)] {
45+
let hopefully_invalid = 123456789_i32;
46+
assert!(super::confstr(hopefully_invalid, hint).is_err());
47+
}
48+
}

0 commit comments

Comments
 (0)