Bug 14832
| Summary: | futimens (AT_FDCWD, arg) should fail | ||
|---|---|---|---|
| Product: | File System | Reporter: | Eric Blake (eblake) |
| Component: | Other | Assignee: | fs_other |
| Status: | CLOSED OBSOLETE | ||
| Severity: | normal | CC: | alan |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | Subsystem: | ||
| Regression: | No | Bisected commit-id: | |
(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface). On Fri, 18 Dec 2009 16:08:29 GMT bugzilla-daemon@bugzilla.kernel.org wrote: > http://bugzilla.kernel.org/show_bug.cgi?id=14832 > > Summary: futimens (AT_FDCWD, arg) should fail > Product: File System > Version: 2.5 > Platform: All > OS/Version: Linux > Tree: Mainline > Status: NEW > Severity: normal > Priority: P1 > Component: Other > AssignedTo: fs_other@kernel-bugs.osdl.org > ReportedBy: ebb9@byu.net > Regression: No > > > According to POSIX 2008, futimens() shall fail with EBADF if fd is not a > valid > file descriptor. AT_FDCWD, being negative, falls into this category. Yet > this > program mistakenly changes the timestamps of the current directory, then > aborts: > > #include <sys/stat.h> > #include <fcntl.h> > #include <errno.h> > #include <stdlib.h> > int > main () > { > if (futimens (AT_FDCWD, NULL) != -1 || errno != EBADF) > abort (); > } > > Since futimens is a library call on top of utimensat, the fix is to add a > special case to the utimensat syscall that fails with EBADF if fd is negative > when path is NULL. > Confused. : long do_utimes(int dfd, char __user *filename, struct timespec *times, int flags) : { : int error = -EINVAL; : : if (times && (!nsec_valid(times[0].tv_nsec) || : !nsec_valid(times[1].tv_nsec))) { : goto out; : } : : if (flags & ~AT_SYMLINK_NOFOLLOW) : goto out; : : if (filename == NULL && dfd != AT_FDCWD) { : struct file *file; : : if (flags & AT_SYMLINK_NOFOLLOW) : goto out; : : file = fget(dfd); : error = -EBADF; : if (!file) : goto out; : afacit, if filename==NULL and dfd==-1 then fget() will return NULL and the syscall returns -EBAFD. Your report doesn't tell us what kernel version you're testing. We did fix a few things ni this area, but it was a long time ago. According to Andrew Morton on 12/22/2009 3:31 PM: > Confused. > > : long do_utimes(int dfd, char __user *filename, struct timespec *times, int > flags) > : { > : int error = -EINVAL; > : > : if (times && (!nsec_valid(times[0].tv_nsec) || > : !nsec_valid(times[1].tv_nsec))) { > : goto out; > : } > : > : if (flags & ~AT_SYMLINK_NOFOLLOW) > : goto out; > : > : if (filename == NULL && dfd != AT_FDCWD) { > : struct file *file; > : > : if (flags & AT_SYMLINK_NOFOLLOW) > : goto out; > : > : file = fget(dfd); > : error = -EBADF; > : if (!file) > : goto out; > : > > afacit, if filename==NULL and dfd==-1 then fget() will return NULL and > the syscall returns -EBAFD. Yes, that's true if dfd==-1. But My complaint was not about dfd==-1, but about dfd==AT_FDCWD, in which case, this block of code is skipped, and you end up operating on the current directory ".". My point is that the line: if (filename == NULL && dfd != AT_FDCWD) { should probably be: if (filename == NULL) { assuming that fget(AT_FDCWD) likewise fails. > > Your report doesn't tell us what kernel version you're testing. We did > fix a few things ni this area, but it was a long time ago. Yesterday, Ulrich patched glibc to avoid the issue from the library side of things (I first raised the report against glibc 2 months ago, along with a proposed patch: http://sources.redhat.com/bugzilla/show_bug.cgi?id=10992). But there is still the case of newer kernels and unpatched glibc, where it would also be nice to patch the kernel. I reproduced the bug on 2.6.31.6-166.fc12.i686, where this test (part of coreutils' configure script) failed with status 2: | #include <fcntl.h> | #include <sys/stat.h> | #include <unistd.h> | | int | main () | { | struct timespec ts[2] = { { 1, UTIME_OMIT }, { 1, UTIME_NOW } }; | int fd = creat ("conftest.file", 0600); | struct stat st; | if (fd < 0) return 1; | if (futimens (AT_FDCWD, NULL)) return 2; | if (futimens (fd, ts)) return 3; | sleep (1); | ts[0].tv_nsec = UTIME_NOW; | ts[1].tv_nsec = UTIME_OMIT; | if (futimens (fd, ts)) return 4; | if (fstat (fd, &st)) return 5; | if (st.st_ctime < st.st_atime) return 6; | | ; | return 0; | } According to Andrew Morton on 12/22/2009 3:31 PM: > Confused. > > : long do_utimes(int dfd, char __user *filename, struct timespec *times, int > flags) > : { > : int error = -EINVAL; > : > : if (times && (!nsec_valid(times[0].tv_nsec) || > : !nsec_valid(times[1].tv_nsec))) { > : goto out; > : } > : > : if (flags & ~AT_SYMLINK_NOFOLLOW) > : goto out; > : > : if (filename == NULL && dfd != AT_FDCWD) { > : struct file *file; > : > : if (flags & AT_SYMLINK_NOFOLLOW) > : goto out; > : > : file = fget(dfd); > : error = -EBADF; > : if (!file) > : goto out; > : > > afacit, if filename==NULL and dfd==-1 then fget() will return NULL and > the syscall returns -EBAFD. Yes, that's true if dfd==-1. But My complaint was not about dfd==-1, but about dfd==AT_FDCWD, in which case, this block of code is skipped, and you end up operating on the current directory ".". My point is that the line: if (filename == NULL && dfd != AT_FDCWD) { should probably be: if (filename == NULL) { assuming that fget(AT_FDCWD) likewise fails. > > Your report doesn't tell us what kernel version you're testing. We did > fix a few things ni this area, but it was a long time ago. Yesterday, Ulrich patched glibc to avoid the issue from the library side of things (I first raised the report against glibc 2 months ago, along with a proposed patch: http://sources.redhat.com/bugzilla/show_bug.cgi?id=10992). But there is still the case of newer kernels and unpatched glibc, where it would also be nice to patch the kernel. I reproduced the bug on 2.6.31.6-166.fc12.i686, where this test (part of coreutils' configure script) failed with status 2: | #include <fcntl.h> | #include <sys/stat.h> | #include <unistd.h> | | int | main () | { | struct timespec ts[2] = { { 1, UTIME_OMIT }, { 1, UTIME_NOW } }; | int fd = creat ("conftest.file", 0600); | struct stat st; | if (fd < 0) return 1; | if (futimens (AT_FDCWD, NULL)) return 2; | if (futimens (fd, ts)) return 3; | sleep (1); | ts[0].tv_nsec = UTIME_NOW; | ts[1].tv_nsec = UTIME_OMIT; | if (futimens (fd, ts)) return 4; | if (fstat (fd, &st)) return 5; | if (st.st_ctime < st.st_atime) return 6; | | ; | return 0; | } Eric Blake <ebb9@byu.net> writes: >> afacit, if filename==NULL and dfd==-1 then fget() will return NULL and >> the syscall returns -EBAFD. > > Yes, that's true if dfd==-1. But My complaint was not about dfd==-1, but > about dfd==AT_FDCWD, in which case, this block of code is skipped, and you > end up operating on the current directory ".". My point is that the line: > if (filename == NULL && dfd != AT_FDCWD) { > should probably be: > if (filename == NULL) { > assuming that fget(AT_FDCWD) likewise fails. No, it changes utimensat() behavior. If error was EBADF, it seems fd shouldn't be AT_FDCWD. So, this behavior would be right thing as utimensat(). [EBADF] The path argument does not specify an absolute path and the fd argument is neither AT_FDCWD nor a valid file descriptor open for reading or searching. >> Your report doesn't tell us what kernel version you're testing. We did >> fix a few things ni this area, but it was a long time ago. > > Yesterday, Ulrich patched glibc to avoid the issue from the library side > of things (I first raised the report against glibc 2 months ago, along > with a proposed patch: > http://sources.redhat.com/bugzilla/show_bug.cgi?id=10992). But there is > still the case of newer kernels and unpatched glibc, where it would also > be nice to patch the kernel. I reproduced the bug on > 2.6.31.6-166.fc12.i686, where this test (part of coreutils' configure > script) failed with status 2: |
According to POSIX 2008, futimens() shall fail with EBADF if fd is not a valid file descriptor. AT_FDCWD, being negative, falls into this category. Yet this program mistakenly changes the timestamps of the current directory, then aborts: #include <sys/stat.h> #include <fcntl.h> #include <errno.h> #include <stdlib.h> int main () { if (futimens (AT_FDCWD, NULL) != -1 || errno != EBADF) abort (); } Since futimens is a library call on top of utimensat, the fix is to add a special case to the utimensat syscall that fails with EBADF if fd is negative when path is NULL.