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.
> >
