use `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)` as a `TMPDIR` fallback on Darwin by thomcc · Pull Request #100824 · rust-lang/rust

thomcc · 2022-08-20T23:28:50Z

Followup to #100196 ~~but should wait on rust-lang/libc#2883 before anything is done with it.~~ ~~(Edit: Now it needs rust-lang/libc#2931... sigh)~~

See #100824 (comment) for an explanation of the change this makes and why I think this is worth doing.

Closes #99608.

rust-highfive · 2022-08-20T23:28:53Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

thomcc · 2022-08-20T23:31:02Z

This is a behavioral change so should go through FCP probably.

See #100196 (comment) and #100196 (comment) for some of the motivation.

I'll also clean up the #[cfg(target_vendor = "apple")] to be the OSes in question and move it out of a draft PR, when I make the change following the API being available in libc.

bors · 2022-09-29T08:30:40Z

☔ The latest upstream changes (presumably #102450) made this pull request unmergeable. Please resolve the merge conflicts.

thomcc · 2022-09-29T13:14:45Z

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:

This is better for security, and is in line with the platform security recommendations, as it is unavailable to other users (although it is the same value as seen by all other processes run by the same user).
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.

rustbot · 2022-09-29T13:14:50Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

bors · 2022-10-24T14:07:39Z

☔ The latest upstream changes (presumably #103471) made this pull request unmergeable. Please resolve the merge conflicts.

…n darwin

bors · 2022-12-06T01:09:31Z

☔ The latest upstream changes (presumably #105328) made this pull request unmergeable. Please resolve the merge conflicts.

edmorley · 2023-11-13T11:04:01Z

@thomcc Hi! We would love to see this land - is this in an FCP queue, or does it need to be manually submitted to that process?

dtolnay · 2024-01-26T03:08:00Z

@rust-lang/libs-api:
@rfcbot fcp merge

Relevant reading:

env::temp_dir returns /private/tmp on Apple instead while /tmp is #100196 (comment)
env::temp_dir returns /private/tmp on Apple instead while /tmp is #100196 (comment)
Consider returning /private/tmp instead of /tmp on macOS with std::env::temp_dir() #99608 (comment)

Link 2 is compelling as to why this PR is the best solution. Link 1 lists our other options, neither of which is better.

rfcbot · 2024-01-26T03:08:02Z

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot · 2024-02-20T16:00:32Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2024-03-01T16:03:35Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

dtolnay

I'll also clean up the #[cfg(target_vendor = "apple")] to be the OSes in question and move it out of a draft PR, when I make the change following the API being available in libc.

Waiting for this (quoted from #100824 (comment)).

LGTM as soon as the cfg is fixed.

Dylan-DPC · 2024-07-25T06:45:45Z

@thomcc any updates on this? thanks

madsmtm

Bump @thomcc?

I can rebase, fix the remaining #[cfg(target_vendor = "...")] and submit an updated PR if you want?

@rustbot label O-apple

Dylan-DPC · 2024-10-10T15:09:40Z

@madsmtm yes feel free to do that :)

madsmtm

I've submitted #131505

madsmtm · 2024-10-10T17:06:43Z

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

+    // 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()) };
+    }


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.

Unsure about the loop but the retry is also done by Swift's implementation of this: https://github.com/swiftlang/swift-corelibs-foundation/blob/2d23cf3dc07951ed2b988608d08d7a54cc53b26e/Sources/Foundation/NSPathUtilities.swift#L38

…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

…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

…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

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

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 20, 2022

rust-highfive assigned m-ou-se Aug 20, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2022

thomcc added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 20, 2022

thomcc force-pushed the darwin_user_temp_dir branch from ee1c648 to ebc6401 Compare September 29, 2022 08:51

thomcc force-pushed the darwin_user_temp_dir branch 3 times, most recently from d294b1f to 49dbfa9 Compare September 29, 2022 13:02

thomcc marked this pull request as ready for review September 29, 2022 13:14

thomcc added 2 commits November 22, 2022 02:23

Bump libc used by std to 0.2.134

f8577b2

use confstr(_CS_DARWIN_USER_TEMP_DIR, ...) as a TMPDIR fallback o…

f8768db

…n darwin

thomcc force-pushed the darwin_user_temp_dir branch from 49dbfa9 to f8768db Compare November 22, 2022 10:24

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 22, 2022

This comment was marked as off-topic.

Sign in to view

rustbot removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 18, 2023

edmorley mentioned this pull request Nov 13, 2023

Consider returning /private/tmp instead of /tmp on macOS with std::env::temp_dir() #99608

Closed

dtolnay assigned dtolnay and unassigned m-ou-se Jan 26, 2024

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 26, 2024

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 20, 2024

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 1, 2024

apiraino removed the to-announce Announce this issue on triage meeting label Mar 7, 2024

dtolnay requested changes Mar 11, 2024

View reviewed changes

madsmtm reviewed Oct 10, 2024

View reviewed changes

rustbot added the O-apple Operating system: Apple / Darwin (macOS, iOS, tvOS, visionOS, watchOS) label Oct 10, 2024

madsmtm mentioned this pull request Oct 10, 2024

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

Merged

madsmtm reviewed Oct 10, 2024

View reviewed changes

bors closed this in f860f5b Nov 23, 2024

Uh oh!

Conversation

thomcc commented Aug 20, 2022 • edited by dtolnay Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Aug 20, 2022

Uh oh!

thomcc commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Sep 29, 2022

Uh oh!

thomcc commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 29, 2022

Uh oh!

bors commented Oct 24, 2022

Uh oh!

bors commented Dec 6, 2022

Uh oh!

This comment was marked as off-topic.

edmorley commented Nov 13, 2023

Uh oh!

dtolnay commented Jan 26, 2024

Uh oh!

rfcbot commented Jan 26, 2024 • edited by Amanieu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfcbot commented Feb 20, 2024

Uh oh!

rfcbot commented Mar 1, 2024

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

Dylan-DPC commented Jul 25, 2024

Uh oh!

madsmtm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dylan-DPC commented Oct 10, 2024

Uh oh!

madsmtm left a comment

Choose a reason for hiding this comment

Uh oh!

madsmtm Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

BlackHoleFox Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

thomcc commented Aug 20, 2022 •

edited by dtolnay

Loading

thomcc commented Aug 20, 2022 •

edited

Loading

thomcc commented Sep 29, 2022 •

edited

Loading

rfcbot commented Jan 26, 2024 •

edited by Amanieu

Loading

madsmtm left a comment •

edited

Loading