[PATCH 0/3] xfsprogs: minor 4.20 fixups

[PATCH 0/3] xfsprogs: minor 4.20 fixups

[Eric Sandeen]

Address a few nitpicks picked up by code scanners, 2 of which
are "new" in 4.20.

[PATCH 1/3] xfs_io: don't pass negative len to copy_file_range_cmd

[Eric Sandeen]

If copy_src_filesize returns an error (-1) we should return that
error, and not pass it to copy_file_range_cmd().

Addresses-Coverity-ID: 1431684 ("Improper use of negative value")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index 4e2969c..bc891c9 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -121,6 +121,10 @@ copy_range_f(int argc, char **argv)
 
 	if (src == 0 && dst == 0 && len == 0) {
 		len = copy_src_filesize(fd);
+		if (len < 0) {
+			close(fd);
+			return 0;
+		}
 		copy_dst_truncate();
 	}

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

[Eric Sandeen]

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

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;
 

Re: [PATCH 1/3] xfs_io: don't pass negative len to copy_file_range_cmd

[Darrick J. Wong]

On Tue, Feb 19, 2019 at 05:17:49PM -0600, Eric Sandeen wrote:
> If copy_src_filesize returns an error (-1) we should return that
> error, and not pass it to copy_file_range_cmd().
> 
> Addresses-Coverity-ID: 1431684 ("Improper use of negative value")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index 4e2969c..bc891c9 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -121,6 +121,10 @@ copy_range_f(int argc, char **argv)
>  
>  	if (src == 0 && dst == 0 && len == 0) {
>  		len = copy_src_filesize(fd);

len is size_t but this function returns off64_t, so if src happened to
be larger than 4GB on a 32-bit system we'll just rip the upper bits off
the number and use that as the file size.

Granted I don't know that we care about 32-bit systems.

> +		if (len < 0) {

But that doesn't fix the problem, because len is size_t, which is
unsigned, so this test is never true.

> +			close(fd);
> +			return 0;
> +		}
>  		copy_dst_truncate();

Ugh, nobody checked the return value of copy_dst_truncate, so if we
can't truncate the destination file we just ignore that and keep
going...

>  	}

...totally untested patch fixing all that nonsense below.

--D

From: Darrick J. Wong <darrick.wong@oracle.com>
Subject: [PATCH] xfs_io: actually check copy file range helper return values

We need to check the return value of copy_src_filesize and
copy_dst_truncate because either could return -1 due to fstat/ftruncate
failure.

Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
Cc: schumaker.anna@gmail.com
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 io/copy_file_range.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index 4e2969c9..ed6fafb1 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -120,11 +120,26 @@ copy_range_f(int argc, char **argv)
 		return 0;
 
 	if (src == 0 && dst == 0 && len == 0) {
-		len = copy_src_filesize(fd);
-		copy_dst_truncate();
+		off64_t	sz;
+
+		sz = copy_src_filesize(fd);
+		if (sz < 0) {
+			ret = sz;
+			goto out;
+		}
+		if ((unsigned long)sz > SIZE_MAX) {
+			ret = -EOVERFLOW;
+			goto out;
+		}
+		len = sz;
+
+		ret = copy_dst_truncate();
+		if (ret < 0)
+			goto out;
 	}
 
 	ret = copy_file_range_cmd(fd, &src, &dst, len);
+out:
 	close(fd);
 	return ret;
 }

[PATCH 3/3] libhandle: zero terminate fspath string

[Eric Sandeen]

I had a heck of a time chasing through whether we were null terminated
by now or not, so just be safe and do it explicitly.

Addresses-Coverity-ID: 1297520 ("Buffer not null terminated")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/libhandle/handle.c b/libhandle/handle.c
index eb099f4..d3927a3 100644
--- a/libhandle/handle.c
+++ b/libhandle/handle.c
@@ -108,6 +108,7 @@ path_to_fshandle(
 
 		fdhp->fsfd = fd;
 		strncpy(fdhp->fspath, fspath, sizeof(fdhp->fspath));
+		fdhp->fspath[sizeof(fdhp->fspath) - 1] = '\0';
 		memcpy(fdhp->fsh, *fshanp, FSIDSIZE);
 
 		fdhp->fnxt = fdhash_head;

Re: [PATCH 1/3] xfs_io: don't pass negative len to copy_file_range_cmd

[Eric Sandeen]

On 2/19/19 5:24 PM, Darrick J. Wong wrote:
> On Tue, Feb 19, 2019 at 05:17:49PM -0600, Eric Sandeen wrote:
>> If copy_src_filesize returns an error (-1) we should return that
>> error, and not pass it to copy_file_range_cmd().
>>
>> Addresses-Coverity-ID: 1431684 ("Improper use of negative value")
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
>> index 4e2969c..bc891c9 100644
>> --- a/io/copy_file_range.c
>> +++ b/io/copy_file_range.c
>> @@ -121,6 +121,10 @@ copy_range_f(int argc, char **argv)
>>  
>>  	if (src == 0 && dst == 0 && len == 0) {
>>  		len = copy_src_filesize(fd);
> 
> len is size_t but this function returns off64_t, so if src happened to
> be larger than 4GB on a 32-bit system we'll just rip the upper bits off
> the number and use that as the file size.
> 
> Granted I don't know that we care about 32-bit systems.
> 
>> +		if (len < 0) {
> 
> But that doesn't fix the problem, because len is size_t, which is
> unsigned, so this test is never true.
> 
>> +			close(fd);
>> +			return 0;
>> +		}
>>  		copy_dst_truncate();
> 
> Ugh, nobody checked the return value of copy_dst_truncate, so if we
> can't truncate the destination file we just ignore that and keep
> going...
> 
>>  	}
> 
> ...totally untested patch fixing all that nonsense below.
> 
> --D
> 
> From: Darrick J. Wong <darrick.wong@oracle.com>
> Subject: [PATCH] xfs_io: actually check copy file range helper return values
> 
> We need to check the return value of copy_src_filesize and
> copy_dst_truncate because either could return -1 due to fstat/ftruncate
> failure.
> 
> Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
> Cc: schumaker.anna@gmail.com
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  io/copy_file_range.c |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index 4e2969c9..ed6fafb1 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -120,11 +120,26 @@ copy_range_f(int argc, char **argv)
>  		return 0;
>  
>  	if (src == 0 && dst == 0 && len == 0) {
> -		len = copy_src_filesize(fd);
> -		copy_dst_truncate();
> +		off64_t	sz;
> +
> +		sz = copy_src_filesize(fd);
> +		if (sz < 0) {
> +			ret = sz;

urgh this is the "what do we return in xfs_io functions?" dilemma,
but every other failure in this function returns 0 to the caller...


> +			goto out;
> +		}
> +		if ((unsigned long)sz > SIZE_MAX) {

as mentioned on IRC this still has the same truncation
problem on a 32-bit box.

-Eric

> +			ret = -EOVERFLOW;
> +			goto out;
> +		}
> +		len = sz;
> +
> +		ret = copy_dst_truncate();
> +		if (ret < 0)

ditto here, ret -1 or 0?

> +			goto out;
>  	}
>  
>  	ret = copy_file_range_cmd(fd, &src, &dst, len);
> +out:
>  	close(fd);
>  	return ret;
>  }
> 

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

[Dave Chinner]

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

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

[Eric Sandeen]

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

[PATCH v2] xfs_io: actually check copy file range helper return values

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

We need to check the return value of copy_src_filesize and
copy_dst_truncate because either could return -1 due to fstat/ftruncate
failure.

Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
Cc: schumaker.anna@gmail.com
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: fix return values and overflow problems
---
 io/copy_file_range.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index 4e2969c9..d069e5bb 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -120,11 +120,24 @@ copy_range_f(int argc, char **argv)
 		return 0;
 
 	if (src == 0 && dst == 0 && len == 0) {
-		len = copy_src_filesize(fd);
-		copy_dst_truncate();
+		off64_t	sz;
+
+		sz = copy_src_filesize(fd);
+		if (sz < 0 || (unsigned long long)sz > SIZE_MAX) {
+			ret = 1;
+			goto out;
+		}
+		len = sz;
+
+		ret = copy_dst_truncate();
+		if (ret < 0) {
+			ret = 1;
+			goto out;
+		}
 	}
 
 	ret = copy_file_range_cmd(fd, &src, &dst, len);
+out:
 	close(fd);
 	return ret;
 }

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

[Darrick J. Wong]

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

Re: [PATCH 3/3] libhandle: zero terminate fspath string

[Darrick J. Wong]

On Tue, Feb 19, 2019 at 05:33:46PM -0600, Eric Sandeen wrote:
> I had a heck of a time chasing through whether we were null terminated
> by now or not, so just be safe and do it explicitly.
> 
> Addresses-Coverity-ID: 1297520 ("Buffer not null terminated")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/libhandle/handle.c b/libhandle/handle.c
> index eb099f4..d3927a3 100644
> --- a/libhandle/handle.c
> +++ b/libhandle/handle.c
> @@ -108,6 +108,7 @@ path_to_fshandle(
>  
>  		fdhp->fsfd = fd;
>  		strncpy(fdhp->fspath, fspath, sizeof(fdhp->fspath));
> +		fdhp->fspath[sizeof(fdhp->fspath) - 1] = '\0';
>  		memcpy(fdhp->fsh, *fshanp, FSIDSIZE);
>  
>  		fdhp->fnxt = fdhash_head;
> 

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

[Eric Sandeen]

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

Re: [PATCH v2] xfs_io: actually check copy file range helper return values

[Anna Schumaker]

On Wed, 2019-02-20 at 09:16 -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We need to check the return value of copy_src_filesize and
> copy_dst_truncate because either could return -1 due to fstat/ftruncate
> failure.

Makes sense.

Reviewed-by: Anna Schumaker <Anna.Schumaker@Netapp.com>

> 
> Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
> Cc: schumaker.anna@gmail.com
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: fix return values and overflow problems
> ---
>  io/copy_file_range.c |   17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index 4e2969c9..d069e5bb 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -120,11 +120,24 @@ copy_range_f(int argc, char **argv)
>  		return 0;
>  
>  	if (src == 0 && dst == 0 && len == 0) {
> -		len = copy_src_filesize(fd);
> -		copy_dst_truncate();
> +		off64_t	sz;
> +
> +		sz = copy_src_filesize(fd);
> +		if (sz < 0 || (unsigned long long)sz > SIZE_MAX) {
> +			ret = 1;
> +			goto out;
> +		}
> +		len = sz;
> +
> +		ret = copy_dst_truncate();
> +		if (ret < 0) {
> +			ret = 1;
> +			goto out;
> +		}
>  	}
>  
>  	ret = copy_file_range_cmd(fd, &src, &dst, len);
> +out:
>  	close(fd);
>  	return ret;
>  }