-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Description
I mentioned in #9342 that a TOCTOU race was introduced. Here is a simplified version of that change to demonstrate the issue:
if src.is_dir() {
/* Special error message. */
}
/* At this point, SRC may no longer be a directory. */
link(src, dest);
Using strace we can see that link is never called. Instead, we fail after a call to statx:
$ C_ALL=C strace uu_ln dir link
[...]
statx(3, "", AT_STATX_SYNC_AS_STAT|AT_EMPTY_PATH, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID|STATX_SUBVOL, stx_attributes=0, stx_mode=S_IFREG|0644, stx_size=2278, ...}) = 0
read(3, "ln-about = Make links between fi"..., 2278) = 2278
read(3, "", 32) = 0
[...]
write(2, "ln", 2ln) = 2
write(2, ": ", 2: ) = 2
write(2, "dir: hard link not allowed for d"..., 40dir: hard link not allowed for directory) = 40
write(2, "\n", 1
) = 1
[...]
This is incorrect, since after the call to statx and before the link, another process may remove src and/or create a file in it's place. If it is removed then the error message is wrong. If a new file is created then the link should be created successfully instead of failing.
Here is the behavior of GNU coreutils:
$ LC_ALL=C strace ln dir link
[...]
linkat(AT_FDCWD, "dir", AT_FDCWD, "link", 0) = -1 EPERM (Operation not permitted)
newfstatat(AT_FDCWD, "dir", {st_mode=S_IFDIR|0755, st_size=0, ...}, AT_SYMLINK_NOFOLLOW) = 0
fcntl(1, F_GETFL) = 0x80002 (flags O_RDWR|O_CLOEXEC)
write(2, "ln: ", 4ln: ) = 4
write(2, "dir: hard link not allowed for d"..., 40dir: hard link not allowed for directory) = 40
write(2, "\n", 1
) = 1
[...]
So we do not skip calling link. If it fails, we use a fstat only as an attempt to provide better diagnostics to the user.
The tac program has a similar issue with Path::is_dir(). Here is uutil's behavior:
$ LC_ALL=C strace uu_tac /
statx(AT_FDCWD, "/", AT_STATX_SYNC_AS_STAT, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID|STATX_SUBVOL, stx_attributes=STATX_ATTR_MOUNT_ROOT, stx_mode=S_IFDIR|0555, stx_size=180, ...}) = 0
write(2, "tac", 3tac) = 3
write(2, ": ", 2: ) = 2
write(2, "/: read error: Invalid argument", 31/: read error: Invalid argument) = 31
write(2, "\n", 1
) = 1
It is also strange, in my opinion, to say there was a read error without opening a file descriptor and calling read.
Here is GNU's behavior:
$ LC_ALL=C strace tac /
[...]
openat(AT_FDCWD, "/", O_RDONLY) = 3
lseek(3, 0, SEEK_END) = 180
ioctl(3, TCGETS2, 0x7ffc9a9cc750) = -1 ENOTTY (Inappropriate ioctl for device)
lseek(3, 0, SEEK_SET) = 0
read(3, 0x559eaee8e361, 8192) = -1 EISDIR (Is a directory)
fcntl(1, F_GETFL) = 0x80002 (flags O_RDWR|O_CLOEXEC)
write(2, "tac: ", 5tac: ) = 5
write(2, "/: read error", 13/: read error) = 13
write(2, ": Is a directory", 16: Is a directory) = 16
write(2, "\n", 1
) = 1
[...]
Here we actually perform a read and trust it's error instead of relying on stat.