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@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxx>
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@xxxxxxxxx
Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
---
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;
}
