Re: [Xen-devel] [PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[PATCH v3 10/10] libxl: fix caller of libxl_cpupool 
functions"):
> Coverity complains cpupool_info leaks a string in failure path. Instead
> of fixing that path, we rely on the callers (two public APIs at the
> moment) of cpupool_info correctly call libxl_cpupoolinfo_dispose in
> their failure path to dispose of any resources.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 38aff8d..02b4a26 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -771,8 +771,11 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, 
> int *nb_pool_out)
>  
>      poolid = 0;
>      for (i = 0;; i++) {
> -        if (cpupool_info(gc, &info, poolid, false))
> +        libxl_cpupoolinfo_init(&info);
> +        if (cpupool_info(gc, &info, poolid, false)) {
> +            libxl_cpupoolinfo_dispose(&info);
>              break;
> +        }

I'm not convinced by this change.

I think that this function has broken error handling: if cpupool_info
fails, it simply ignores the error.

To fix this, it would have to clean up the array properly.

Furthermore, I don't understand why it uses the temporary variable
`info'.  Surely it could have cpupool_info write directly into the
right array slot.  If it did that, then the error handling path would
work to free up any failure.


By putting both these fixes in the same patch, you have denied me the
opportunity to ack the other change, which is fine ...

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel