On 3/14/19 4:04 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Reviewed-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
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;
> }
>
