| Message ID | acf1deea-a637-571b-e501-c7cbcde95c38@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfsprogs: minor 4.20 fixups | expand |
On Tue, Feb 19, 2019 at 05:23:38PM -0600, Eric Sandeen wrote: > openfile() stats a path to determine whether it is a pipe, and then > opens it with flags based on the type it saw during the stat. It's > possible that the file at that path changes in between, and Coverity > points this out. > > Instead, always open O_NONBLOCK, stat the fd we got, and turn the > flag back off via fcntl() if it's not a pipe. > > Addresses-Coverity-ID: 1442788 ("Time of check time of use") For O_NONBLOCK I think there is zero harm in the code as it stands, but I don't really care about it tht much. I do wonder what happens if need to conditionally use O_NONBLOCK on open(), though.... > @@ -112,6 +106,22 @@ openfile( > } > } > > + if (fstat(fd, &st) < 0) { > + perror("stat"); > + close(fd); > + return -1; > + } > + > + /* We only want to keep nonblocking behavior for pipes */ > + if (!S_ISFIFO(st.st_mode)) { > + oflags &= ~O_NONBLOCK; > + if (fcntl(fd, F_SETFL, oflags) < 0) { Don't you need to do oflags = fcntl(fd, F_GETFL, NULL) first? > + perror("fcntl"); > + close(fd); > + return -1; > + } can you add a "goto out_close;" error jump to this function now that this is the fourth and fifth places that have this same close/return on error... CHeers, Dave.
On 2/19/19 7:50 PM, Dave Chinner wrote: > On Tue, Feb 19, 2019 at 05:23:38PM -0600, Eric Sandeen wrote: >> openfile() stats a path to determine whether it is a pipe, and then >> opens it with flags based on the type it saw during the stat. It's >> possible that the file at that path changes in between, and Coverity >> points this out. >> >> Instead, always open O_NONBLOCK, stat the fd we got, and turn the >> flag back off via fcntl() if it's not a pipe. >> >> Addresses-Coverity-ID: 1442788 ("Time of check time of use") > > For O_NONBLOCK I think there is zero harm in the code as it stands, > but I don't really care about it tht much. > > I do wonder what happens if need to conditionally use O_NONBLOCK on > open(), though.... > >> @@ -112,6 +106,22 @@ openfile( >> } >> } >> >> + if (fstat(fd, &st) < 0) { >> + perror("stat"); >> + close(fd); >> + return -1; >> + } >> + >> + /* We only want to keep nonblocking behavior for pipes */ >> + if (!S_ISFIFO(st.st_mode)) { >> + oflags &= ~O_NONBLOCK; >> + if (fcntl(fd, F_SETFL, oflags) < 0) { > > Don't you need to do oflags = fcntl(fd, F_GETFL, NULL) first? maybe? We just opened it with oflags, but *shrug* I guess it doesn't hurt? > >> + perror("fcntl"); >> + close(fd); >> + return -1; >> + } > > can you add a "goto out_close;" error jump to this function now that > this is the fourth and fifth places that have this same close/return > on error... yeah, good point. > CHeers, > > Dave. >
On Tue, Feb 19, 2019 at 10:41:57PM -0600, Eric Sandeen wrote: > > > On 2/19/19 7:50 PM, Dave Chinner wrote: > > On Tue, Feb 19, 2019 at 05:23:38PM -0600, Eric Sandeen wrote: > >> openfile() stats a path to determine whether it is a pipe, and then > >> opens it with flags based on the type it saw during the stat. It's > >> possible that the file at that path changes in between, and Coverity > >> points this out. > >> > >> Instead, always open O_NONBLOCK, stat the fd we got, and turn the > >> flag back off via fcntl() if it's not a pipe. > >> > >> Addresses-Coverity-ID: 1442788 ("Time of check time of use") > > > > For O_NONBLOCK I think there is zero harm in the code as it stands, > > but I don't really care about it tht much. > > > > I do wonder what happens if need to conditionally use O_NONBLOCK on > > open(), though.... > > > >> @@ -112,6 +106,22 @@ openfile( > >> } > >> } > >> > >> + if (fstat(fd, &st) < 0) { > >> + perror("stat"); > >> + close(fd); > >> + return -1; > >> + } > >> + > >> + /* We only want to keep nonblocking behavior for pipes */ > >> + if (!S_ISFIFO(st.st_mode)) { > >> + oflags &= ~O_NONBLOCK; > >> + if (fcntl(fd, F_SETFL, oflags) < 0) { > > > > Don't you need to do oflags = fcntl(fd, F_GETFL, NULL) first? > > maybe? We just opened it with oflags, but *shrug* I guess it doesn't > hurt? The kernel can set O_ flags on the file (e.g. O_LARGEFILE) for you, which means that if you try to F_SETFL with the original oflags, the kernel will think you're trying to clear those flags and misunderstand. --D > > > >> + perror("fcntl"); > >> + close(fd); > >> + return -1; > >> + } > > > > can you add a "goto out_close;" error jump to this function now that > > this is the fourth and fifth places that have this same close/return > > on error... > > yeah, good point. > > > CHeers, > > > > Dave. > >
On 2/20/19 11:23 AM, Darrick J. Wong wrote: >>> Don't you need to do oflags = fcntl(fd, F_GETFL, NULL) first? >> maybe? We just opened it with oflags, but *shrug* I guess it doesn't >> hurt? > The kernel can set O_ flags on the file (e.g. O_LARGEFILE) for you, > which means that if you try to F_SETFL with the original oflags, the > kernel will think you're trying to clear those flags and misunderstand. Ok, thanks. -Eric
diff --git a/io/open.c b/io/open.c index f5fbd2c..856018b 100644 --- a/io/open.c +++ b/io/open.c @@ -62,6 +62,12 @@ openfile( int oflags; oflags = flags & IO_READONLY ? O_RDONLY : O_RDWR; + /* + * In case we've been passed a pipe to open, don't block waiting for a + * reader or writer to appear. We want to either succeed or error out + * immediately. We'll clear O_NONBLOCK if it's not a pipe. + */ + oflags |= O_NONBLOCK; if (flags & IO_APPEND) oflags |= O_APPEND; if (flags & IO_CREAT) @@ -81,18 +87,6 @@ openfile( if (flags & IO_NOFOLLOW) oflags |= O_NOFOLLOW; - /* - * if we've been passed a pipe to open, don't block waiting for a - * reader or writer to appear. We want to either succeed or error out - * immediately. - */ - if (stat(path, &st) < 0 && errno != ENOENT) { - perror("stat"); - return -1; - } - if (S_ISFIFO(st.st_mode)) - oflags |= O_NONBLOCK; - fd = open(path, oflags, mode); if (fd < 0) { if (errno == EISDIR && @@ -112,6 +106,22 @@ openfile( } } + if (fstat(fd, &st) < 0) { + perror("stat"); + close(fd); + return -1; + } + + /* We only want to keep nonblocking behavior for pipes */ + if (!S_ISFIFO(st.st_mode)) { + oflags &= ~O_NONBLOCK; + if (fcntl(fd, F_SETFL, oflags) < 0) { + perror("fcntl"); + close(fd); + return -1; + } + } + if (!geom || !platform_test_xfs_fd(fd)) return fd;
openfile() stats a path to determine whether it is a pipe, and then opens it with flags based on the type it saw during the stat. It's possible that the file at that path changes in between, and Coverity points this out. Instead, always open O_NONBLOCK, stat the fd we got, and turn the flag back off via fcntl() if it's not a pipe. Addresses-Coverity-ID: 1442788 ("Time of check time of use") Signed-off-by: Eric Sandeen <sandeen@redhat.com> ---