Should std::env::set_current_dir be unsafe?

[fweimer]

The current directory is a per-process resource (at least on POSIX). Changing it could affect what shared objects are loaded by a library, and therefore, there might be an indirect impact on memory safety. Should it be marked unsafe?

[sfackler]

Any API that loads libraries at runtime and pulls symbols from them is inherently unsafe for a multitude of reasons unrelated to set_current_directory.

[fweimer]

@sfackler, loading from the current directory is safe if you know what this directory is because you just called set_current_dir. The load path vulnerabilities on Windows and other systems would not apply as a result.

The reference to library loading was an attempt at a formal argument towards lack of memory safety for set_current_dir. Practically speaking, I think it is more important that when processing command line arguments (or files read from the current directory), you typically have a dependency on the current directory of the process. But currently, some other thread can easily call set_current_dir to another value temporarily (perhaps in preparation of starting another process), and there is currently no indication that this is a very bad thing to do.

[sfackler]

You are always going to be asserting that some symbol is a function with a certain signature or a static of a certain type.

In any case, the convention is to place the unsafety at the closest layer to where the actual badness could result. It is safe to get a raw pointer from a slice, but not safe to dereference it, for example.

It is also unreasonable to make a function unsafe after it is already stable (and has been for over a year).

[retep998]

@fweimer If you want to load a specific library, use an absolute path! I can't think of any time when you absolutely must use a relative path to the current directory. Get the current directory once when your process first starts up and then use absolute paths from then on.

[fweimer]

@sfackler Is it really impossible already to correct past mistakes? Would it make sense to add a warning to the documentation? Or do you disagree that the issue exists at all?

[retep998]

set_current_dir is not unsafe because it does not directly cause any memory unsafety. The only unsafety that can result is from other unsafe code assuming the current directory will not change, in which case it is that code's fault for assuming the current directory won't change, and not set_current_dir's fault.

The only way I could see set_current_dir needing unsafe is if it modifies the current directory without any sort of synchronization, but I'm pretty sure chdir is synchronized and I know SetCurrentDirectory definitely is.

[fweimer]

@retep998 There is no common object to synchronize on, though. If a function receives a relative pathname (or gets it from the command line or the environment), it has to perform pathname resolution based on the current directory. If there is anything else in the process which temporarily alters the current directory for any purpose, this is not reliable and can give incorrect results. And because there is no obvious lock to take around the current directory change, it will always be risky to call this function.

The conclusions I and others (Rich Felker in particular, with his library-safe functions concepts) is that library code cannot call chdir, ever. I was therefore surprised to see it included in the Rust standard library, without further comment.

[retep998]

@fweimer That's not memory unsafe. Rust has very specific rules about what is considered unsafe, and setting the current directory is not unsafe by those rules. It's a simple atomic operation to either get or set the current directory. As long as the safety of unsafe code never relies on the incorrect assumption that the current directory will never change, then everything is perfectly fine. If unsafe code ever does make that assumption, then it is that code's fault for doing so

As I said before, it is relatively simple to avoid depending on the current directory. Anything that can take a relative path can also take an absolute path, so just stick to absolute paths everywhere. You can get the current directory once when your program first starts up, cache the result, and then every time you get a relative path as input you can just make it absolute using that current directory you cached. That way if some other code changes the current directory you won't be affected.

[fweimer]

@retep998 I was following this precedent:

static mut var: u32 = 0;

fn main() {
   var = 5;
}

This does not compile:

error[E0133]: use of mutable static requires unsafe function or block
 --> t.rs:4:4
  |
4 |    var = 5;
  |    ^^^ use of mutable static

This cannot cause memory safety violations on its own, either.

[hanna-kruppe]

It can (at least, the general feature of mutating statics) -- accesses from multiple threads cause data races, which cause UB, which can in turn cause anything including memory safety violations.

[retep998]

Meanwhile set_current_dir corresponds to chdir and SetCurrentDirectory and I know for sure that SetCurrentDirectory does not have any data races. Every time it gets or sets the current directory it uses an internal lock. chdir is likely the same.

[vadimcn]

A similar problem exists with std::env::set_var and, really, any function that changes the global state (including modification of files). We can't possibly make them all "unsafe".
But I agree, it would be prudent to add a warning in documentation for set_current_dir.