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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5061 bytes --]

On 17.06.20 20:27,

Sorry :/

Connor Kuehl wrote:
> 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>
> ---
>  block.c                    |  4 ++++
>  tests/qemu-iotests/298     | 47 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/298.out |  5 ++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 57 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 6dbcb7e083..b335d6bcb2 100644
> --- a/block.c
> +++ b/block.c
> @@ -6116,6 +6116,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;
> +        }

Looks good.

>      }
>  
>      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..1e30caebec
> --- /dev/null
> +++ b/tests/qemu-iotests/298
> @@ -0,0 +1,47 @@
> +#!/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):
> +        actual = iotests.qemu_img_pipe(

(pylint complains about the indentation that follows, it says it’d
prefer four spaces less.  Make of that want you wish, AFAIA there’s no
requirement to get a clean pylint pass for iotests yet. O:))

> +                'create',
> +                '-f', 'qcow2',

This should be iotests.imgfmt, not specifically qcow2.

> +                '-b', '',
> +                '/tmp/foo'

I’m not sure I like using /tmp/foo very much, for two reasons: First,
this expects that the backing filename will be checked before something
really happens with the image filename; for example on Windows, /tmp/foo
is just not a valid filename, so we might end up getting an error
because of that.  Or, for me, I like saving mails in a directory called
/tmp/foo, so maybe at some point I get an error because /tmp/foo already
exists as a directory.

Second, if we ever do run into a regression or some configuration where
this test unexpectedly manages to create the image, it would overwrite
/tmp/foo.  Which maybe isn’t good, because maybe it’s a file the user
wants to keep around.

The fix is simple, just use some file under iotests.test_dir.  I think
the easiest way right now is something like:

with iotests.FilePath('test.img') as img_path:
    actual = iotests.qemu_img_pipe('create', ..., img_path, '1M')
    expected = f'qemu-img: {img_path}: Expected...'
    self.assertEqual(...)

(On that note, I’d give a size to qemu-img, so that the invocation is
correct apart from the missing backing file name.)

> +        )
> +        expected = 'qemu-img: /tmp/foo: Expected backing file name,' \
> +                   ' got empty string'
> +
> +        self.assertEqual(actual.strip(), expected.strip())
> +
> +
> +if __name__ == '__main__':
> +    iotests.main(supported_fmts=['raw', 'qcow2'])

raw isn’t supported, this test only works for formats that support
backing files.  (Which also means it should work for qed and qcow, I
imagine...?)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]