-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
touch: remove metadata() call to avoid potential TOCTOU before File::create. #10061
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
base: main
Are you sure you want to change the base?
Conversation
|
GNU testsuite comparison: |
|
Guess my assumption of (os error XX) being OK in the string was wrong as it's causing these to fail. Some of the failing tests don't align with GNU either: On main branch, I get this: with my PR, it has this behavior: The test is failing because it's looking for the behavior of what's currently in the main branch, but I don't think it's in line with GNU coreutil's error message. Edited to add this test was done with GNU coreutils 9.9 from Arch Linux. |
|
GNU testsuite comparison: |
Set the default line width to 72 for all page headers in `pr`, regardless of whether a custom date format is being used. Before, a single space was used to separate the three components of the header (date, filename, and page number) if a custom date format was not given.
|
GNU testsuite comparison: |
…ut GNU coreutils uses "setting times of" error message.
|
GNU testsuite comparison: |
| if opts.source == Source::Now && opts.date.is_none() { | ||
| return Ok(()); | ||
| // Use OpenOptions to create the file if it doesn't exist. This is used in lieu of | ||
| // stat() to avoid TOCTOU issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add this word to the ignore list
| // Originally, the metadata stat() in touch_file() would catch "no create", | ||
| // but since it was removed to fix TOCTOU issues, we need to handle it here. | ||
| if let Err(ref e) = result { | ||
| if opts.no_create && e.kind() != ErrorKind::NotADirectory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that the logic for handling --no-create with ELOOP (dangling symlinks) is inverted. With --no-create, ELOOP should return Ok(()), not error out
no?
| InputFile::Path(path) => (Cow::Borrowed(path), false), | ||
| }; | ||
| touch_file(&path, is_stdout, opts, atime, mtime).map_err(|e| { | ||
| if let Err(e) = touch_file(&path, is_stdout, opts, atime, mtime).map_err(|e| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe using a match statement could simplify this code ?
|
|
||
| // err_good_cause indicates that the error is not one we should ignore | ||
| if err_good_cause { | ||
| return Err(TouchError::TouchFileError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating TouchError::TouchFileError inside touch_file() leads to double-wrapping since the caller also wraps it. maybe return UResult instead.
| // but since it was removed to fix TOCTOU issues, we need to handle it here. | ||
| if let Err(ref e) = result { | ||
| if opts.no_create && e.kind() != ErrorKind::NotADirectory { | ||
| #[cfg(any(unix, target_os = "macos"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cfg checks use redundant any(unix, target_os = "macos") instead of just unix (macOS is already Unix)
|
a few files are conflicting |
Fixes #10019
The
metadata()function calls stat at the OS level, which was being checked beforeFile::create()causing a potential race condition between the stat'd file and the opened file (i.e., Time-of-check/time-of-usage problem). This PR usesOpenOptionsinstead with options to turn off O_TRUNC. It also uses additional flags (O_NOCTTY and O_NONBLOCK) on UNIX targets usingOpenOptionsExt. These are POSIX compliant options, so I usedcfg(unix)rather thantarget = "linux". I also had to handle ENXIO separately as touching a FIFO should be fine, but with O_NONBLOCK, this will cause an error. However, the error is not something we care about as it's just telling us it couldn't do anything with the FIFO, so for UNIX targets, that error is ignored like it is for GNU coreutils.now uutils (after PR):
stat has been removed as a system call and
opennow is used to both create and test file errors. To align the error messages with GNU coreutils, such as permission denied, this PR adds an error message to the 'touch-error-cannot-touch' error.main branch:
after PR:
I did not see it necessary to remove the (os error 13), but can easily be done, but I see it in other utilities, such as install, so I figured it could be left here too.