Re: [PATCH 1/2] qemu: monitor:Prevent a NULl pointer from being accessed

[Peter Krempa]

On Wed, Feb 12, 2020 at 22:09:42 +0800, Yi Wang wrote:
> From: Huang Zijiang <huang.ziji...@zte.com.cn>
> 
> virJSONValueObjectGetObject maybe return NULL if the key is
> missing or if value is not the correct TYPE, so we have to prevent
> a NULl pointer from being accessed.

If you see below we already check that the function will not return NULL
before using it. Is there any bug you are attempting to fix? If yes
please also describe the symptoms of it.

> Signed-off-by: Huang Zijiang <huang.ziji...@zte.com.cn>
> Signed-off-by: Yi Wang <wang.y...@zte.com.cn>
> ---
>  src/qemu/qemu_monitor_json.c | 89 
> +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index e5164d2..51b40e0 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1697,7 +1697,12 @@ qemuMonitorJSONGetStatus(qemuMonitorPtr mon,
>          goto cleanup;
>  
>      data = virJSONValueObjectGetObject(reply, "return");
> -
> +    if (!data){
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("query-status reply was missing return data"));
> +        return -1;
> +    }
> +  

Let's have some more context of this function:

   if (!(cmd = qemuMonitorJSONMakeCommand("query-status", NULL)))
       return -1;

   if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
       goto cleanup;

   if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
       goto cleanup;

   data = virJSONValueObjectGetObject(reply, "return");

   if (virJSONValueObjectGetBoolean(data, "running", running) < 0) {
       virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

As you can see qemuMonitorJSONCheckReply is used which makes sure that
the 'return' object exists in 'reply'. This means that
virJSONValueObjectGetObject will return a valid pointer.

This was deliberately cleaned up this way some time ago.

>      if (virJSONValueObjectGetBoolean(data, "running", running) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("query-status reply was missing running state"));
> @@ -6027,6 +6078,11 @@ qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon,
>          return -1;
>  
>      data = virJSONValueObjectGetObject(reply, "return");
> +    if (!data) {
> +            (VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("query-cpu-model-baseline reply was missing 
> return data"));

This won't even compile.


> +            return -1;
> +     }