Re: [Qemu-devel] [PATCH] i386: Fix arch_query_cpu_model_expansion() leak

[Paolo Bonzini]

On 16/08/2018 20:35, Eduardo Habkost wrote:
> Reported by Coverity:
> 
> Error: RESOURCE_LEAK (CWE-772): [#def439]
> qemu-2.12.0/target/i386/cpu.c:3179: alloc_fn: Storage is returned from 
> allocation function "qdict_new".
> qemu-2.12.0/qobject/qdict.c:34:5: alloc_fn: Storage is returned from 
> allocation function "g_malloc0".
> qemu-2.12.0/qobject/qdict.c:34:5: var_assign: Assigning: "qdict" = 
> "g_malloc0(4120UL)".
> qemu-2.12.0/qobject/qdict.c:37:5: return_alloc: Returning allocated memory 
> "qdict".
> qemu-2.12.0/target/i386/cpu.c:3179: var_assign: Assigning: "props" = storage 
> returned from "qdict_new()".
> qemu-2.12.0/target/i386/cpu.c:3217: leaked_storage: Variable "props" going 
> out of scope leaks the storage it points to.
> 
> This was introduced by commit b8097deb359b ("i386: Improve
> query-cpu-model-expansion full mode").
> 
> The leak is only theoretical: if ret->model->props is set to
> props, the qapi_free_CpuModelExpansionInfo() call will free props
> too in case of errors.  The only way for this to not happen is if
> we enter the default branch of the switch statement, which would
> never happen because all CpuModelExpansionType values are being
> handled.
> 
> It's still worth to change this to make the allocation logic
> easier to follow and make the Coverity error go away.  To make
> everything simpler, initialize ret->model and ret->model->props
> earlier in the function.
> 
> While at it, remove redundant check for !prop because prop is
> always initialized at the beginning of the function.
> 
> Fixes: b8097deb359bbbd92592b9670adfe9e245b2d0bd
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
>  target/i386/cpu.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 723e02221e..e23c05c4e1 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3758,6 +3758,9 @@ arch_query_cpu_model_expansion(CpuModelExpansionType 
> type,
>      }
>  
>      props = qdict_new();
> +    ret->model = g_new0(CpuModelInfo, 1);
> +    ret->model->props = QOBJECT(props);
> +    ret->model->has_props = true;
>  
>      switch (type) {
>      case CPU_MODEL_EXPANSION_TYPE_STATIC:
> @@ -3778,15 +3781,9 @@ arch_query_cpu_model_expansion(CpuModelExpansionType 
> type,
>          goto out;
>      }
>  
> -    if (!props) {
> -        props = qdict_new();
> -    }
>      x86_cpu_to_dict(xc, props);
>  
> -    ret->model = g_new0(CpuModelInfo, 1);
>      ret->model->name = g_strdup(base_name);
> -    ret->model->props = QOBJECT(props);
> -    ret->model->has_props = true;
>  
>  out:
>      object_unref(OBJECT(xc));
> 

Queued, thanks.

Paolo