diff mbox series

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

Message ID 20200811212318.708290-1-ckuehl@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v2] block: Raise an error when backing file parameter is an empty string | expand

Commit Message

Connor Kuehl Aug. 11, 2020, 9:23 p.m. UTC
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>
---
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 ++++
 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

Comments

Kevin Wolf Aug. 12, 2020, 6:58 a.m. UTC | #1
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
Connor Kuehl Aug. 13, 2020, 12:59 p.m. UTC | #2
On 8/12/20 1:58 AM, Kevin Wolf wrote:
> 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.
> 
> 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. :-)

Hi Kevin! Thanks for the review :-)

I think it'd be best for my own edification to address your comments 
here instead of applying this now. I'll send a v3.

Connor

> 
> Kevin
>
diff mbox series

Patch

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'])
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