Patchwork [5/5] monitor: Catch printing to non-existent monitor

login
register
mail settings
Submitter Luiz Capitulino
Date Dec. 14, 2009, 8:53 p.m.
Message ID <1260824004-2941-6-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/41139/
State New
Headers show

Comments

Luiz Capitulino - Dec. 14, 2009, 8:53 p.m.
The monitor_vprintf() function now touches the 'mon' pointer
before calling monitor_puts(), this causes block migration
to segfault as its functions call monitor_printf() with a
NULL 'mon'.

To fix the problem this commit moves the 'mon' NULL check
from monitor_puts() to monitor_vprintf().

This can potentially hide bugs, but for some reason this has
been the behavior for a long time.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Markus Armbruster - Dec. 15, 2009, 9:42 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> The monitor_vprintf() function now touches the 'mon' pointer
> before calling monitor_puts(), this causes block migration
> to segfault as its functions call monitor_printf() with a
> NULL 'mon'.

I figure this worked fine until commit 4a29a85d made monitor_vprintf()
dereference mon.

> To fix the problem this commit moves the 'mon' NULL check
> from monitor_puts() to monitor_vprintf().
>
> This can potentially hide bugs, but for some reason this has
> been the behavior for a long time.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index b518cc4..ebd0282 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -177,9 +177,6 @@ static void monitor_puts(Monitor *mon, const char *str)
>  {
>      char c;
>  
> -    if (!mon)
> -        return;
> -
>      for(;;) {
>          c = *str++;
>          if (c == '\0')
> @@ -195,6 +192,9 @@ static void monitor_puts(Monitor *mon, const char *str)
>  
>  void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>  {
> +    if (!mon)
> +        return;
> +
>      if (mon->mc && !mon->mc->print_enabled) {
>          qemu_error_new(QERR_UNDEFINED_ERROR);
>      } else {

There are no other callers of monitor_puts(), so removing the check
there is okay.

Before the code motion, we throw QERR_UNDEFINED_ERROR on
monitor_vprintf(NULL, ...).  Afterwards, we don't.  Could you explain
why that's okay?
Luiz Capitulino - Dec. 15, 2009, 12:02 p.m.
On Tue, 15 Dec 2009 10:42:46 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Before the code motion, we throw QERR_UNDEFINED_ERROR on
> monitor_vprintf(NULL, ...).  Afterwards, we don't.  Could you explain
> why that's okay?

 We never did that. A call like that will just segfault QEMU
w/o this fix.
Markus Armbruster - Dec. 15, 2009, 1:56 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 15 Dec 2009 10:42:46 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Before the code motion, we throw QERR_UNDEFINED_ERROR on
>> monitor_vprintf(NULL, ...).  Afterwards, we don't.  Could you explain
>> why that's okay?
>
>  We never did that. A call like that will just segfault QEMU
> w/o this fix.

You're right.  Brain fart on my part.

Patch

diff --git a/monitor.c b/monitor.c
index b518cc4..ebd0282 100644
--- a/monitor.c
+++ b/monitor.c
@@ -177,9 +177,6 @@  static void monitor_puts(Monitor *mon, const char *str)
 {
     char c;
 
-    if (!mon)
-        return;
-
     for(;;) {
         c = *str++;
         if (c == '\0')
@@ -195,6 +192,9 @@  static void monitor_puts(Monitor *mon, const char *str)
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
+    if (!mon)
+        return;
+
     if (mon->mc && !mon->mc->print_enabled) {
         qemu_error_new(QERR_UNDEFINED_ERROR);
     } else {