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

env::temp_dir returns /private/tmp on Apple instead while /tmp is #100196

Closed
wants to merge 2 commits into from

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Aug 6, 2022

a symlink in fact.

ref #99608

@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2022

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

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 6, 2022
@rust-highfive
Copy link
Collaborator

r? @thomcc

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2022
@thomcc
Copy link
Member

thomcc commented Aug 7, 2022

Hmm, I am not so sure about this.

The preferred tempdir on these platforms is the value from libc::confstr(libc::_CS_DARWIN_USER_TEMP_DIR, ...), although this will be a user-specific directory with a name like /var/folders/rw/gsmfnk2s12d34hywmktc28q40000gn/T/, where the specific name will change over time and is unpredictable.

This is best for security, as it avoids providing a temporary directory that another user (who may be at a different privilege level) has access to, unlike /tmp or /private/tmp. Usually $TMPDIR is set to this, but I guess not always? That said, I don't know if using this is a change we'd want to make.

More generally, we've always returned /tmp on darwin platforms, despite it (AFAIK) always being a symlink to /private/tmp. So I don't really think changing it to this is what I'd recommend, If we're going to change it, I'd rather see us use the safer directory.

Either way, this is a behavioral change that would require documentation to be updated, and t-libs-api signoff.

@rustbot label +T-libs-api -T-libs

@rustbot rustbot 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 7, 2022
@thomcc
Copy link
Member

thomcc commented Aug 7, 2022

I think this needs further discussion in the t-libs-api meeting. Essentially there are three choices here for what to do if $TMPDIR is unset on Darwin platforms:

  1. Keep the same behavior we have currently, which is to return /tmp, which is (and has always been on these platforms) a symlink to /private/tmp.

  2. Change to returning /private/tmp. This avoids returning something that is symlink, and returns the target of the symlink. The rationale here is certain programs may not expect the path returned to be a symlink (although I'm not totally certain that it's never a symlink on other unixes, and IMO we don't want to promise that it is not a symlink -- or if we do we should canonicalize it).

  3. Change to returning the string by confstr(_CS_DARWIN_USER_TEMP_DIR, ...). This matches the behavior used when $TMPDIR is provided (it usually points here), avoids some security pitfalls, and matches the behavior recommended by apple for these platforms. This will be a string like /var/folders/rw/gsmfnk2s12d34hywmktc28q40000gn/T/ which varies between users (but not between processes of the same user) and periodically is randomized around every 3 days and/or on boot.

I recommend either the third (which is a behavioral change, but we don't promise this to be unchanging, and I would have expected $TMPDIR to usually be set) or the first, which keeps the current behavior.

@thomcc thomcc closed this Aug 7, 2022
@thomcc thomcc reopened this Aug 7, 2022
@thomcc
Copy link
Member

thomcc commented Aug 7, 2022

(Sorry for the accidental close -- accidentally hit tab). See previous message for explanation of my nomination.

@rustbot label +I-libs-api-nominated

@rustbot rustbot added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Aug 7, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Aug 9, 2022

The rationale here is certain programs may not expect the path returned to be a symlink

Are there any examples of this? Is there any realistic use case where a symlink would be a problem?

@m-ou-se m-ou-se removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Aug 16, 2022
@thomcc
Copy link
Member

thomcc commented Aug 18, 2022

It did cause an issue for someone using SQLite, since they were requesting SQLite not follow symbolic links. I don't particularly care about this.

That said, I don't think most software on Darwin OSes should be using /tmp or /private/tmp in most cases. Doing so goes against Apple's security guidelines, which tell you to use confstr(_CS_DARWIN_USER_TEMP_DIR, ...) instead.

Generally this is not needed, as most of the time $TMPDIR is set in the environment to the directory the confstr invocation will resolve to. I guess in some cases it is not though, and think that we should use that as a fallback instead if we can justify the behavioral change.

len: libc::size_t,
) -> libc::size_t;
}
let tmpdir = unsafe { libc::getenv(b"TMPDIR".as_ptr() as *const libc::c_char) };
Copy link
Member

Choose a reason for hiding this comment

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

We already checked the env above. Also, don't use libc::getenv as it doesn't hold the env lock. Also, this string isn't NUL-terminated.

let tmpdir = unsafe { libc::getenv(b"TMPDIR".as_ptr() as *const libc::c_char) };
if tmpdir.is_null() {
let mut buf: Vec<u8> = vec![0; libc::PATH_MAX as usize];
const _CS_DARWIN_USER_TEMP_DIR: libc::c_int = 65537;
Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for rust-lang/libc#2883.

}
let tmpdir = unsafe { libc::getenv(b"TMPDIR".as_ptr() as *const libc::c_char) };
if tmpdir.is_null() {
let mut buf: Vec<u8> = vec![0; libc::PATH_MAX as usize];
Copy link
Member

Choose a reason for hiding this comment

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

50 bytes should be enough in practice, but really this should have some retry logic. I was going to submit thomcc@ee1c648 as a PR at one point, but you can just integrate the changes from it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it s better if you do since you authored all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants