Return error for current_exe on nonexistent file #69557
Conversation
The underlying implementation of current_exe uses the symlink at /proc/self/exe to find its own executable path. Deleting or replacing the original executable results in a PathBuf containing the original path with " (deleted)" appended effectively pointing to a nonexistent file Signed-off-by: Cristian Kubis <cristian.kubis@tsunix.de>
|
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Signed-off-by: Cristian Kubis <cristian.kubis@tsunix.de>
|
Adding extra syscalls is something that feels suspicious; so randomly reassigning (hopefully to someone more knowledgable!)... r? @KodrAus |
| Ok(path) => { | ||
| let path_str = path.to_str().ok_or(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "path contains invalid UTF-8 data", |
There was a problem hiding this comment.
This seems likely to be incorrect... we shouldn't need to require a path to be UTF-8; that's the entire reason we have Path(Buf) and OsStr(ing).
| "path contains invalid UTF-8 data", | ||
| ))?; | ||
| unsafe { | ||
| if libc::access(path_str.as_ptr() as *const i8, libc::F_OK) == 0 { |
There was a problem hiding this comment.
Does this FFI call require a NUL-terminated string? Rust strings are not.
There was a problem hiding this comment.
Either way lstat is the better way to test if the path indicated exists or not; access checks if the current user can also read the path. Though this gets weird if you do create a file with the (deleted) suffix on in; this will detect it as existing, but the link is actually broken (/proc is, probably not surprisingly, magic here). I think stat("/proc/self/exe") may actually be a better solution for checking if the path exists.
I don't know; there's no good fool-proof way to do this; we're just trying to dodge the throwing knives someone is sending our way at this point.
There was a problem hiding this comment.
@clarfon outlined a possible approach, but that takes at least 4 syscalls. #69343 (comment)
I'm not sure if that's worth it because under race conditions paths must be considered stale the very moment you get them from a syscall. The only race-free way to do these things is to operate on file descriptors instead of paths.
There was a problem hiding this comment.
Yup, although I will say that the number of syscalls for this seems incredibly reasonable for an API that will be called extremely infrequently (likely seconds between invocations).
There was a problem hiding this comment.
Note that once the symlink is broken due to the target being deleted, it is always broken, so it's at least a one-way gate here. But that cannot rely on readlink followed by use of the path; it must be gotten from stat("/proc/self/exe") directly.
|
In general, this change doesn't seem to solve the problem, it only introduces another time-of-check to time-of-use race condition (which already exists). The caller of this function can still get the same error as we only return a You should be able to see that with a test like |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@shepmaster You are right that it won't fix TOCTOU and that wasn't my intention either. My problem with the current behavior is that the function can return That being said you are right that you would need to perform additional checks to avoid TOCTOU problems later in your code anyway which makes this redundent. Its highly debatable whether being "more semantically correct" is worth an additional syscall. |
Ah, my mistake. The question still remains about the overall usefulness of this change, but I can’t speak to it. |
|
Probably the right way to do this would be to do a |
|
oh wait that doesn’t return the filename… Maybe canonicalize the |
|
This check would introduce a false negative when the current process does not have search permissions for one of the directories in the path, which is not the same as the file not existing. This may be relevant when passing the path to a setuid or sending it to a differently privileged process via IPC. |
JohnCSimon
added
S-waiting-on-author
|
Ping from Triage: @tsurai Any updates? |
|
I've stopped working on this as I'm waiting for the lib subteam to decide wether this is a bug, expected behavior or a wontfix because of the introduction of an additional syscall. |
JohnCSimon
added
the
S-blocked
Dylan-DPC-zz
added
S-waiting-on-team
|
@rustbot modify labels to +T-libs +I-nominated |
|
Error: Label I-nominated can only be set by Rust team members Please let |
|
@rustbot modify labels to +T-libs |
rustbot
added
the
T-libs-api
|
There are many ways in which the string returned by
In light of this I feel that we should simply update the documentation of @rfcbot fcp close |
|
Team member @Amanieu has proposed to close 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
added
proposed-final-comment-period
|
One use case of However on Linux it might be better to use Perhaps we could add something like |
rfcbot
added
the
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
removed
the
proposed-final-comment-period
|
Why would that need to be something in the stdlib? Sounds like a great use for a crate though. FWIW, my usual use case is to discover the location of the executable in order to derive paths relative to it for resources or whatnot. Typically used in tests since assuming it is not deleted is fine there or the path manipulation is still valid even with |
|
Potential other option -- why not just deprecate |
|
I think the documentation is on the right track for that. But I do think wording that it is just a path and that the path itself, even if fetched correctly, may not be the real path that was used to launch the process due to various reasons. Maybe just clarifying that "security implications" includes things like "supporting reexec" or other examples. |
|
@clarfon I don't think Rust should deprecate anything unless:
|
|
@Kixunil This has been done in the past with A good question to ask is why people want this. Is it to read the running binary? To provide a name for the current command in help? Those use cases should be covered outside libstd imho |
|
@clarfon another crate existing satisfies "a better method to achieve the goal". :) |
|
The final comment period, with a disposition to close, 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. |
rfcbot
added
finished-final-comment-period
|
So, according to the fcp disposition above, i'm gonna go ahead and close this. |
The underlying implementation of
current_exeon linux uses the symlink at/proc/self/exeto find its own executable path. Deleting or replacing the original executable results in a PathBuf containing the original path with" (deleted)"appended effectively pointing to a nonexistent file.This change only tests the existence of the file not whether the user can actually access it.
Fix #69343