diff mbox series

[07/36] xfs_io: actually check copy file range helper return values

Message ID 155259746648.31886.15984241616376190830.stgit@magnolia (mailing list archive)
State Accepted, archived
Headers show
Series xfsprogs-5.0: fix various problems | expand

Commit Message

Darrick J. Wong March 14, 2019, 9:04 p.m. UTC 
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")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 io/copy_file_range.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Eric Sandeen March 15, 2019, 2:12 a.m. UTC | #1 
On 3/14/19 4:04 PM, 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.
> 
> Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

I see this has reviews already, but I'm not conviced 1 is the right
return on error.

i.e. the error condition just prior returns 0:

        fd = openfile(argv[optind], NULL, IO_READONLY, 0, NULL);
        if (fd < 0)
                return 0;

but OTOH if copy_file_range_cmd() fails we return nonzero.  Argh.

Is there a rhyme or reason here that I'm missing?  Is it broken now
so just do a coinflip on the new return for now?


> ---
>  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;
>  }
>
Darrick J. Wong March 15, 2019, 2:56 a.m. UTC | #2 
On Thu, Mar 14, 2019 at 09:12:11PM -0500, Eric Sandeen wrote:
> On 3/14/19 4:04 PM, 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.
> > 
> > Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> I see this has reviews already, but I'm not conviced 1 is the right
> return on error.
> 
> i.e. the error condition just prior returns 0:
> 
>         fd = openfile(argv[optind], NULL, IO_READONLY, 0, NULL);
>         if (fd < 0)
>                 return 0;
> 
> but OTOH if copy_file_range_cmd() fails we return nonzero.  Argh.
> 
> Is there a rhyme or reason here that I'm missing?  Is it broken now
> so just do a coinflip on the new return for now?

Pretty much. :(

I don't really care if you change the return value, I just thought it
was bad form not to check the syscall/helper return values at all.

--D

> 
> > ---
> >  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;
> >  }
> >
Eric Sandeen March 15, 2019, 4:51 p.m. UTC | #3 
On 3/14/19 9:56 PM, Darrick J. Wong wrote:
> On Thu, Mar 14, 2019 at 09:12:11PM -0500, Eric Sandeen wrote:
>> On 3/14/19 4:04 PM, 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.
>>>
>>> Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>> Reviewed-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>
>> I see this has reviews already, but I'm not conviced 1 is the right
>> return on error.
>>
>> i.e. the error condition just prior returns 0:
>>
>>         fd = openfile(argv[optind], NULL, IO_READONLY, 0, NULL);
>>         if (fd < 0)
>>                 return 0;
>>
>> but OTOH if copy_file_range_cmd() fails we return nonzero.  Argh.
>>
>> Is there a rhyme or reason here that I'm missing?  Is it broken now
>> so just do a coinflip on the new return for now?
> 
> Pretty much. :(
> 
> I don't really care if you change the return value, I just thought it
> was bad form not to check the syscall/helper return values at all.

Yeah no argument there.  Ok, so this function already returns inconsistently,
I'll worry about that later (I hope)

Thanks,
-Eric
Dave Chinner March 17, 2019, 10:45 p.m. UTC | #4 
On Fri, Mar 15, 2019 at 11:51:56AM -0500, Eric Sandeen wrote:
> 
> 
> On 3/14/19 9:56 PM, Darrick J. Wong wrote:
> > On Thu, Mar 14, 2019 at 09:12:11PM -0500, Eric Sandeen wrote:
> >> On 3/14/19 4:04 PM, 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.
> >>>
> >>> Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
> >>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >>> Reviewed-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >>
> >> I see this has reviews already, but I'm not conviced 1 is the right
> >> return on error.
> >>
> >> i.e. the error condition just prior returns 0:
> >>
> >>         fd = openfile(argv[optind], NULL, IO_READONLY, 0, NULL);
> >>         if (fd < 0)
> >>                 return 0;
> >>
> >> but OTOH if copy_file_range_cmd() fails we return nonzero.  Argh.
> >>
> >> Is there a rhyme or reason here that I'm missing?  Is it broken now
> >> so just do a coinflip on the new return for now?
> > 
> > Pretty much. :(
> > 
> > I don't really care if you change the return value, I just thought it
> > was bad form not to check the syscall/helper return values at all.
> 
> Yeah no argument there.  Ok, so this function already returns inconsistently,
> I'll worry about that later (I hope)

I think I've still got a patch sitting around that fixes the
inconsistent return values across most of xfs_io. Oh, I posted it at
one point, too.

https://marc.info/?l=linux-xfs&m=152644986309024&w=2

I guess I should resurrect it at some point.

Cheers,

Dave.
diff mbox series

Patch

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