diff mbox series

cpu: Fix crash with empty -cpu option

Message ID 20190418034501.5038-1-ehabkost@redhat.com
State New
Headers show
Series cpu: Fix crash with empty -cpu option | expand

Commit Message

Eduardo Habkost April 18, 2019, 3:45 a.m. UTC 
Fix the following crash:

  $ qemu-system-x86_64 -cpu ''
  qemu-system-x86_64: qom/cpu.c:291: cpu_class_by_name: \
      Assertion `cpu_model && cc->class_by_name' failed.

Regression test script included.

Fixes: commit 99193d8f2ef5 ("cpu: drop unnecessary NULL check and cpu_common_class_by_name()")
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 exec.c                              |  4 ++++
 tests/acceptance/empty_cpu_model.py | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+)
 create mode 100644 tests/acceptance/empty_cpu_model.py

Comments

Stefano Garzarella April 18, 2019, 8:09 a.m. UTC | #1 
On Thu, Apr 18, 2019 at 12:45:01AM -0300, Eduardo Habkost wrote:
> Fix the following crash:
> 
>   $ qemu-system-x86_64 -cpu ''
>   qemu-system-x86_64: qom/cpu.c:291: cpu_class_by_name: \
>       Assertion `cpu_model && cc->class_by_name' failed.
> 
> Regression test script included.
> 
> Fixes: commit 99193d8f2ef5 ("cpu: drop unnecessary NULL check and cpu_common_class_by_name()")
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  exec.c                              |  4 ++++
>  tests/acceptance/empty_cpu_model.py | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+)
>  create mode 100644 tests/acceptance/empty_cpu_model.py
> 

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Tested-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano
Igor Mammedov April 18, 2019, 11:22 a.m. UTC | #2 
On Thu, 18 Apr 2019 00:45:01 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Fix the following crash:
> 
>   $ qemu-system-x86_64 -cpu ''
>   qemu-system-x86_64: qom/cpu.c:291: cpu_class_by_name: \
>       Assertion `cpu_model && cc->class_by_name' failed.
> 
> Regression test script included.
> 
> Fixes: commit 99193d8f2ef5 ("cpu: drop unnecessary NULL check and cpu_common_class_by_name()")
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  exec.c                              |  4 ++++
>  tests/acceptance/empty_cpu_model.py | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+)
>  create mode 100644 tests/acceptance/empty_cpu_model.py
> 
> diff --git a/exec.c b/exec.c
> index 1ca95df9d8..d816b38863 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -999,6 +999,10 @@ const char *parse_cpu_option(MachineState *machine, const char *cpu_option)
>      const char *cpu_type;
>  
>      model_pieces = g_strsplit(cpu_option, ",", 2);
> +    if (!model_pieces[0]) {
> +        error_report("-cpu option cannot be empty");
> +        exit(1);

s/1/EXIT_FAILURE/

> +    }
>  
>      cc = lookup_cpu_class(model_pieces[0], &error_fatal);
>      cpu_type = object_class_get_name(OBJECT_CLASS(cc));
> diff --git a/tests/acceptance/empty_cpu_model.py b/tests/acceptance/empty_cpu_model.py
> new file mode 100644
> index 0000000000..3f4f663582
> --- /dev/null
> +++ b/tests/acceptance/empty_cpu_model.py
> @@ -0,0 +1,19 @@
> +# Check for crash when using empty -cpu option
> +#
> +# Copyright (c) 2019 Red Hat, Inc.
> +#
> +# Author:
> +#  Eduardo Habkost <ehabkost@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +import subprocess
> +from avocado_qemu import Test
> +
> +class EmptyCPUModel(Test):
> +    def test(self):
> +        cmd = [self.qemu_bin, '-S', '-display', 'none', '-machine', 'none', '-cpu', '']
> +        r = subprocess.run(cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
> +        self.assertEquals(r.returncode, 1, "QEMU exit code should be 1")
> +        self.assertEquals(r.stdout, b'', "QEMU stdout should be empty")
> +        self.assertNotEquals(r.stderr, b'', "QEMU stderr shouldn't be empty")
Eduardo Habkost April 18, 2019, 1:23 p.m. UTC | #3 
On Thu, Apr 18, 2019 at 01:22:16PM +0200, Igor Mammedov wrote:
> On Thu, 18 Apr 2019 00:45:01 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Fix the following crash:
> > 
> >   $ qemu-system-x86_64 -cpu ''
> >   qemu-system-x86_64: qom/cpu.c:291: cpu_class_by_name: \
> >       Assertion `cpu_model && cc->class_by_name' failed.
> > 
> > Regression test script included.
> > 
> > Fixes: commit 99193d8f2ef5 ("cpu: drop unnecessary NULL check and cpu_common_class_by_name()")
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  exec.c                              |  4 ++++
> >  tests/acceptance/empty_cpu_model.py | 19 +++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> >  create mode 100644 tests/acceptance/empty_cpu_model.py
> > 
> > diff --git a/exec.c b/exec.c
> > index 1ca95df9d8..d816b38863 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -999,6 +999,10 @@ const char *parse_cpu_option(MachineState *machine, const char *cpu_option)
> >      const char *cpu_type;
> >  
> >      model_pieces = g_strsplit(cpu_option, ",", 2);
> > +    if (!model_pieces[0]) {
> > +        error_report("-cpu option cannot be empty");
> > +        exit(1);
> 
> s/1/EXIT_FAILURE/

Why?

There are 753 instances of "exit(1)" in the tree, and 171
instances of "exit(EXIT_FAILURE)".
Igor Mammedov April 18, 2019, 3:05 p.m. UTC | #4 
On Thu, 18 Apr 2019 10:23:29 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Apr 18, 2019 at 01:22:16PM +0200, Igor Mammedov wrote:
> > On Thu, 18 Apr 2019 00:45:01 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > Fix the following crash:
> > > 
> > >   $ qemu-system-x86_64 -cpu ''
> > >   qemu-system-x86_64: qom/cpu.c:291: cpu_class_by_name: \
> > >       Assertion `cpu_model && cc->class_by_name' failed.
> > > 
> > > Regression test script included.
> > > 
> > > Fixes: commit 99193d8f2ef5 ("cpu: drop unnecessary NULL check and cpu_common_class_by_name()")
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  exec.c                              |  4 ++++
> > >  tests/acceptance/empty_cpu_model.py | 19 +++++++++++++++++++
> > >  2 files changed, 23 insertions(+)
> > >  create mode 100644 tests/acceptance/empty_cpu_model.py
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index 1ca95df9d8..d816b38863 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -999,6 +999,10 @@ const char *parse_cpu_option(MachineState *machine, const char *cpu_option)
> > >      const char *cpu_type;
> > >  
> > >      model_pieces = g_strsplit(cpu_option, ",", 2);
> > > +    if (!model_pieces[0]) {
> > > +        error_report("-cpu option cannot be empty");
> > > +        exit(1);  
> > 
> > s/1/EXIT_FAILURE/  
> 
> Why?
it's more descriptive in general

> There are 753 instances of "exit(1)" in the tree, and 171
> instances of "exit(EXIT_FAILURE)".
Bad example in past is probably not a good justification,
especially when adding new code (but I'm not going to argue about it).
Eduardo Habkost April 18, 2019, 8:16 p.m. UTC | #5 
On Thu, Apr 18, 2019 at 12:45:01AM -0300, Eduardo Habkost wrote:
> Fix the following crash:
> 
>   $ qemu-system-x86_64 -cpu ''
>   qemu-system-x86_64: qom/cpu.c:291: cpu_class_by_name: \
>       Assertion `cpu_model && cc->class_by_name' failed.
> 
> Regression test script included.
> 
> Fixes: commit 99193d8f2ef5 ("cpu: drop unnecessary NULL check and cpu_common_class_by_name()")
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Queued on machine-next.
diff mbox series

Patch

diff --git a/exec.c b/exec.c
index 1ca95df9d8..d816b38863 100644
--- a/exec.c
+++ b/exec.c
@@ -999,6 +999,10 @@  const char *parse_cpu_option(MachineState *machine, const char *cpu_option)
     const char *cpu_type;
 
     model_pieces = g_strsplit(cpu_option, ",", 2);
+    if (!model_pieces[0]) {
+        error_report("-cpu option cannot be empty");
+        exit(1);
+    }
 
     cc = lookup_cpu_class(model_pieces[0], &error_fatal);
     cpu_type = object_class_get_name(OBJECT_CLASS(cc));
diff --git a/tests/acceptance/empty_cpu_model.py b/tests/acceptance/empty_cpu_model.py
new file mode 100644
index 0000000000..3f4f663582
--- /dev/null
+++ b/tests/acceptance/empty_cpu_model.py
@@ -0,0 +1,19 @@ 
+# Check for crash when using empty -cpu option
+#
+# Copyright (c) 2019 Red Hat, Inc.
+#
+# Author:
+#  Eduardo Habkost <ehabkost@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+import subprocess
+from avocado_qemu import Test
+
+class EmptyCPUModel(Test):
+    def test(self):
+        cmd = [self.qemu_bin, '-S', '-display', 'none', '-machine', 'none', '-cpu', '']
+        r = subprocess.run(cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+        self.assertEquals(r.returncode, 1, "QEMU exit code should be 1")
+        self.assertEquals(r.stdout, b'', "QEMU stdout should be empty")
+        self.assertNotEquals(r.stderr, b'', "QEMU stderr shouldn't be empty")