Re: [PATCH 2/3] xfs_io: fix TOCTOU in openfile()

[Dave Chinner <david@fromorbit.com>]

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.
-- 
Dave Chinner
david@fromorbit.com