The API of `Path::exists()` encourages broken code

41

Marker files are a common counter example. Systemd even has a condition for checking if file exists. And yes, I used it in practice. There's no good reason to do anything with the file if it's only supposed to exist or not exist, encoding one bit of information.

1 Like
42

I'm very much into security and I take races and other things seriously. It's one of the main reasons I use Rust in the first place.

In some cases worrying about TOCTOU bugs is not reasonable. Such as when accessing system files in /usr/share/.... Those can be modified by root only, so if anyone else can modify it the system is already broken.

I have an actual production code that does sanity check upfront to detect misinstallation early. This is very useful for debugging. Any errors encountered later are handled anyway because it's Rust, so I have to. :slight_smile:

I don't see a good reason to keep a vector of filedescriptors around. Especially because some of those file are actually executable - scripts or programs. I don't remember there being any variant of exec that accepts fd instead of path.

1 Like
43

@Gilnaa that wouldn't work because the other process would just point to itself. You could however pass format!("/proc/{}/fd{}", get_pid(), file.as_raw_fd()). But even that is cursed because if the process that passed it dies for any reason and PID gets reused the fd may point to a random file or socket.

@mjw if the process is still alive the user can readlink /proc/... but you're right it sucks.

44

Yeah, it wouldn't to itself, but it also inherited this fd from you.

45

Ah, right, if it's direct child it works. It can cause a lot of trouble if that child also happens to exec other processes. I had insane issues with unclosed file descriptors in the past because Python didn't open with CLOEXEC.

Accepting FD is the right thing to do.

46

FYI the Path::try_exists() PR was approved. Tracking issue

3 Likes
47

Is there any particular reason why stat must be used for these cases instead of lstat?

I'm asking because I'm sketching out a Windows implementation. There the classic (and fast) way of testing for file's existence is to use GetFileAttributes but that's more akin to lstat in that it doesn't follow symlinks. Windows (or at least the Win32 API) does not really have a stat equivalent. You have to actually open the file to follow symlinks, which is slower. But then again the performance of try_exists may not particularly matter.

48

On the work PC (where I do not use Rust, but for his problem it does not matter) we have unfortunatelly set very strict rules about file permissions, so after clonning a repo (if I forget to set permissions for that folder) I get quite a lot of very confusing errors, because IDE does not have permission to create files there. We tried few IDEs over the time, but I am yet to see one that will report permission error, instead of internal crashes, or "UnexpectedException", or other useless errors, when simple "Do not have permission to create file XXX." would be clear, and instantly pointed user to a problem.

2 Likes
49

I've just realised that is_file and is_dir use the same pattern of treating metadata error as false:

impl Path {
    #[stable(feature = "path_ext", since = "1.5.0")]
    pub fn is_file(&self) -> bool {
        fs::metadata(self).map(|m| m.is_file()).unwrap_or(false)
    }
}
1 Like
50

Great point! I even think this breaking may be even worse.

51

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.