fix segfaults caused by accessing CPU in empty machine

[Qemu-devel] [PATCH 1/2] monitor.c: make mon_get_cpu return NULL when there is no CPU

Posted by Ziyue Yang 8 years, 11 months ago

From: Ziyue Yang <yzylivezh@hotmail.com>

Currently mon_get_cpu always dereferences first_cpu without checking
whether it's a valid pointer. This commit adds check before dereferencing,
and reports "No CPU" info if there isn't any CPU then returns NULL.

Signed-off-by: Ziyue Yang <skiver.cloud.yzy@gmail.com>
---
 monitor.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3cd72a9bab..6b25cf7a2b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1026,6 +1026,10 @@ int monitor_set_cpu(int cpu_index)
 CPUState *mon_get_cpu(void)
 {
     if (!cur_mon->mon_cpu) {
+        if (!first_cpu) {
+            monitor_printf(cur_mon, "No CPU available on this machine\n");
+            return NULL;
+        }
         monitor_set_cpu(first_cpu->cpu_index);
     }
     cpu_synchronize_state(cur_mon->mon_cpu);
@@ -2495,11 +2499,11 @@ static int default_fmt_size = 4;
 static int is_valid_option(const char *c, const char *typestr)
 {
     char option[3];
-  
+
     option[0] = '-';
     option[1] = *c;
     option[2] = '\0';
-  
+
     typestr = strstr(typestr, option);
     return (typestr != NULL);
 }
@@ -2864,7 +2868,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
                     p++;
                     if(c != *p) {
                         if(!is_valid_option(p, typestr)) {
-                  
+
                             monitor_printf(mon, "%s: unsupported option -%c\n",
                                            cmd->name, *p);
                             goto fail;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/2] monitor.c: make mon_get_cpu return NULL when there is no CPU

Posted by Philippe Mathieu-Daudé 8 years, 11 months ago

On 02/17/2017 05:27 AM, Ziyue Yang wrote:
> From: Ziyue Yang <yzylivezh@hotmail.com>
>
> Currently mon_get_cpu always dereferences first_cpu without checking
> whether it's a valid pointer. This commit adds check before dereferencing,
> and reports "No CPU" info if there isn't any CPU then returns NULL.
>
> Signed-off-by: Ziyue Yang <skiver.cloud.yzy@gmail.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  monitor.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 3cd72a9bab..6b25cf7a2b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1026,6 +1026,10 @@ int monitor_set_cpu(int cpu_index)
>  CPUState *mon_get_cpu(void)
>  {
>      if (!cur_mon->mon_cpu) {
> +        if (!first_cpu) {
> +            monitor_printf(cur_mon, "No CPU available on this machine\n");
> +            return NULL;
> +        }
>          monitor_set_cpu(first_cpu->cpu_index);
>      }
>      cpu_synchronize_state(cur_mon->mon_cpu);
> @@ -2495,11 +2499,11 @@ static int default_fmt_size = 4;
>  static int is_valid_option(const char *c, const char *typestr)
>  {
>      char option[3];
> -
> +
>      option[0] = '-';
>      option[1] = *c;
>      option[2] = '\0';
> -
> +
>      typestr = strstr(typestr, option);
>      return (typestr != NULL);
>  }
> @@ -2864,7 +2868,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>                      p++;
>                      if(c != *p) {
>                          if(!is_valid_option(p, typestr)) {
> -
> +
>                              monitor_printf(mon, "%s: unsupported option -%c\n",
>                                             cmd->name, *p);
>                              goto fail;
>

Re: [Qemu-devel] [PATCH 1/2] monitor.c: make mon_get_cpu return NULL when there is no CPU

Posted by Thomas Huth 8 years, 11 months ago

On 19.02.2017 04:55, Philippe Mathieu-Daudé wrote:
> On 02/17/2017 05:27 AM, Ziyue Yang wrote:
>> From: Ziyue Yang <yzylivezh@hotmail.com>
>>
>> Currently mon_get_cpu always dereferences first_cpu without checking
>> whether it's a valid pointer. This commit adds check before
>> dereferencing,
>> and reports "No CPU" info if there isn't any CPU then returns NULL.
>>
>> Signed-off-by: Ziyue Yang <skiver.cloud.yzy@gmail.com>
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>> ---
>>  monitor.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 3cd72a9bab..6b25cf7a2b 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1026,6 +1026,10 @@ int monitor_set_cpu(int cpu_index)
>>  CPUState *mon_get_cpu(void)
>>  {
>>      if (!cur_mon->mon_cpu) {
>> +        if (!first_cpu) {
>> +            monitor_printf(cur_mon, "No CPU available on this
>> machine\n");
>> +            return NULL;
>> +        }
>>          monitor_set_cpu(first_cpu->cpu_index);
>>      }
>>      cpu_synchronize_state(cur_mon->mon_cpu);
>> @@ -2495,11 +2499,11 @@ static int default_fmt_size = 4;
>>  static int is_valid_option(const char *c, const char *typestr)
>>  {
>>      char option[3];
>> -
>> +
>>      option[0] = '-';
>>      option[1] = *c;
>>      option[2] = '\0';
>> -
>> +
>>      typestr = strstr(typestr, option);
>>      return (typestr != NULL);
>>  }
>> @@ -2864,7 +2868,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>>                      p++;
>>                      if(c != *p) {
>>                          if(!is_valid_option(p, typestr)) {
>> -
>> +
>>                              monitor_printf(mon, "%s: unsupported
>> option -%c\n",
>>                                             cmd->name, *p);
>>                              goto fail;

Your patch contains some unnecessary white space changes, please try to
avoid that! (or send a separate "beautification" patch to fix these).

 Thomas

Re: [Qemu-devel] [PATCH 1/2] monitor.c: make mon_get_cpu return NULL when there is no CPU

Posted by Yang Ziyue 8 years, 11 months ago

Sorry for forgetting the "no irrelevant change" rule...won't do that next time!

2017-02-20 14:09 GMT+08:00 Thomas Huth <thuth@redhat.com>:
> On 19.02.2017 04:55, Philippe Mathieu-Daudé wrote:
>> On 02/17/2017 05:27 AM, Ziyue Yang wrote:
>>> From: Ziyue Yang <yzylivezh@hotmail.com>
>>>
>>> Currently mon_get_cpu always dereferences first_cpu without checking
>>> whether it's a valid pointer. This commit adds check before
>>> dereferencing,
>>> and reports "No CPU" info if there isn't any CPU then returns NULL.
>>>
>>> Signed-off-by: Ziyue Yang <skiver.cloud.yzy@gmail.com>
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>>> ---
>>>  monitor.c | 10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index 3cd72a9bab..6b25cf7a2b 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -1026,6 +1026,10 @@ int monitor_set_cpu(int cpu_index)
>>>  CPUState *mon_get_cpu(void)
>>>  {
>>>      if (!cur_mon->mon_cpu) {
>>> +        if (!first_cpu) {
>>> +            monitor_printf(cur_mon, "No CPU available on this
>>> machine\n");
>>> +            return NULL;
>>> +        }
>>>          monitor_set_cpu(first_cpu->cpu_index);
>>>      }
>>>      cpu_synchronize_state(cur_mon->mon_cpu);
>>> @@ -2495,11 +2499,11 @@ static int default_fmt_size = 4;
>>>  static int is_valid_option(const char *c, const char *typestr)
>>>  {
>>>      char option[3];
>>> -
>>> +
>>>      option[0] = '-';
>>>      option[1] = *c;
>>>      option[2] = '\0';
>>> -
>>> +
>>>      typestr = strstr(typestr, option);
>>>      return (typestr != NULL);
>>>  }
>>> @@ -2864,7 +2868,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>>>                      p++;
>>>                      if(c != *p) {
>>>                          if(!is_valid_option(p, typestr)) {
>>> -
>>> +
>>>                              monitor_printf(mon, "%s: unsupported
>>> option -%c\n",
>>>                                             cmd->name, *p);
>>>                              goto fail;
>
> Your patch contains some unnecessary white space changes, please try to
> avoid that! (or send a separate "beautification" patch to fix these).
>
>  Thomas
>