Re: [PATCH v2] block: Raise an error when backing file parameter is an empty string

[Kevin Wolf]

Am 11.08.2020 um 23:23 hat Connor Kuehl geschrieben:
> Providing an empty string for the backing file parameter like so:
> 
>       qemu-img create -f qcow2 -b '' /tmp/foo
> 
> allows the flow of control to reach and subsequently fail an assert
> statement because passing an empty string to
> 
>       bdrv_get_full_backing_filename_from_filename()
> 
> simply results in NULL being returned without an error being raised.
> 
> To fix this, let's check for an empty string when getting the value from
> the opts list.
> 
> Reported-by: Attila Fazekas <afazekas@redhat.com>
> Fixes: https://bugzilla.redhat.com/1809553
> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>

In 'qemu-img rebase' we accept an empty string to mean "no backing
file". I guess it would be a bit more consistent to do the same here,
though of course there is no real reason to use -b '' in 'create'. So I
don't really mind an error either, I just wanted to mention it.

> v2:
>   - Removed 4 spaces to resolve pylint warning
>   - Updated format to be 'iotests.imgfmt' instead
>     of hardcoding 'qcow2'
>   - Use temporary file instead of '/tmp/foo'
>   - Give a size parameter to qemu-img
>   - Run test for qcow2, qcow, qed and *not* raw
> 
>  block.c                    |  4 ++++
>  tests/qemu-iotests/298     | 49 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/298.out |  5 ++++

We have multiple series using 298. You were first, though, so in case of
doubt, the others will have to rebase.

>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 59 insertions(+)
>  create mode 100755 tests/qemu-iotests/298
>  create mode 100644 tests/qemu-iotests/298.out
> 
> diff --git a/block.c b/block.c
> index d9ac0e07eb..1f72275b87 100644
> --- a/block.c
> +++ b/block.c
> @@ -6117,6 +6117,10 @@ void bdrv_img_create(const char *filename, const char 
> *fmt,
>                               "same filename as the backing file");
>              goto out;
>          }
> +        if (backing_file[0] == '\0') {
> +            error_setg(errp, "Expected backing file name, got empty string");
> +            goto out;
> +        }
>      }
>  
>      backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
> diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
> new file mode 100755
> index 0000000000..879dae2d8e
> --- /dev/null
> +++ b/tests/qemu-iotests/298
> @@ -0,0 +1,49 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright (C) 2020 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +
> +
> +# Regression test for avoiding an assertion when the 'backing file'
> +# parameter (-b) is set to an empty string for qemu-img create
> +#
> +#   qemu-img create -f qcow2 -b '' /tmp/foo
> +#
> +# This ensures the invalid parameter is handled with a user-
> +# friendly message instead of a failed assertion.
> +
> +import iotests
> +
> +class TestEmptyBackingFilename(iotests.QMPTestCase):
> +
> +
> +    def test_empty_backing_file_name(self):
> +        with iotests.FilePath('test.img') as img_path:
> +            actual = iotests.qemu_img_pipe(
> +                'create',
> +                '-f', iotests.imgfmt,
> +                '-b', '',
> +                img_path,
> +                '1M'
> +            )
> +            expected = f'qemu-img: {img_path}: Expected backing file name,' \
> +                       ' got empty string'
> +
> +            self.assertEqual(actual.strip(), expected.strip())
> +
> +
> +if __name__ == '__main__':
> +    iotests.main(supported_fmts=['qed', 'qcow', 'qcow2'])

This looks like a test case that would be better served by not using
QMPTestCase, but just printing the qemu-img output and having the
message compared against the reference output.

In fact, there is already 049 for testing some qemu-img create options
and we could just add a line there (or multiple lines to cover other
backing file related error cases, too).

Putting it there would both simplify the test code and keep 298 free for
the other series.

> diff --git a/tests/qemu-iotests/298.out b/tests/qemu-iotests/298.out
> new file mode 100644
> index 0000000000..ae1213e6f8
> --- /dev/null
> +++ b/tests/qemu-iotests/298.out
> @@ -0,0 +1,5 @@
> +.
> +----------------------------------------------------------------------
> +Ran 1 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 025ed5238d..6f80c884a1 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -306,6 +306,7 @@
>  295 rw
>  296 rw
>  297 meta
> +298 img auto quick
>  299 auto quick
>  301 backing quick
>  302 quick

None of the above is really a reason to reject the patch. I guess this
is more of a "are you sure? (y/n)" before I apply it. :-)

Kevin