Re: [PATCH for-4.5] tools/libxl: Use of init()/dispose() to avoid leaking libxl_dominfo.ssid_label

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Jan 05, 2015 at 02:19:58PM +0000, Andrew Cooper wrote:
> libxl_dominfo contains a ssid_label pointer which will have memory allocated
> for it in libxl_domain_info() if the hypervisor has CONFIG_XSM compiled.
> However, the lack of appropriate use of libxl_dominfo_{init,dispose}() will
> cause the label string to be leaked, even in success cases.
> 
> This was discovered by XenServers Coverity scanning, and are issues not
> identified by upstream Coverity Scan.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
> 
> Requesting a release-exception as suggested by IanJ.  These are memory leaks
> which accumulate via the successful completion of libxl library functions (if
> XSM is enabled), which will undoubtedly cause issues for the likes of libvirt
> and xenopsd-libxl which use libxl in daemon processes.

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>


> 
> As we are very close to the release, I have opted for the more
> obviously-correct code rather than the pragmatically better code, particularly
> in two locations where libxl_domain_info() is called in a loop, and the
> dispose()/init() pair is required to prevent leaking the allocation on each
> iteration.
> 
> All of the uses of libxl_domain_info() patched here could better be an
> xc_dominfo_getlist() which reduces the quantity of hypercalls made, and the
> amount of data transformation done behind the scenes.
> ---
>  tools/libxl/libxl.c     |   26 ++++++++++++++++++++++++--
>  tools/libxl/libxl_dom.c |   14 ++++++++++----
>  2 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 50a8928..372dd3b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -364,6 +364,8 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
>      char *uuid;
>      const char *vm_name_path;
>  
> +    libxl_dominfo_init(&info);
> +
>      dom_path = libxl__xs_get_dompath(gc, domid);
>      if (!dom_path) goto x_nomem;
>  
> @@ -481,6 +483,7 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
>      rc = 0;
>   x_rc:
>      if (our_trans) xs_transaction_end(ctx->xsh, our_trans, 1);
> +    libxl_dominfo_dispose(&info);
>      return rc;
>  
>   x_fail:  rc = ERROR_FAIL;  goto x_rc;
> @@ -4594,6 +4597,8 @@ static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      uint32_t free_mem_slack_kb = 0;
>  
> +    libxl_dominfo_init(&info);
> +
>  retry_transaction:
>      t = xs_transaction_start(ctx->xsh);
>  
> @@ -4626,6 +4631,8 @@ retry_transaction:
>          }
>      }
>  
> +    libxl_dominfo_dispose(&info);
> +    libxl_dominfo_init(&info);
>      rc = libxl_domain_info(ctx, &info, 0);
>      if (rc < 0)
>          goto out;
> @@ -4664,7 +4671,7 @@ out:
>              rc = ERROR_FAIL;
>      }
>  
> -
> +    libxl_dominfo_dispose(&info);
>      return rc;
>  }
>  
> @@ -4999,6 +5006,8 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs)
>      uint32_t target_memkb = 0;
>      libxl_dominfo info;
>  
> +    libxl_dominfo_init(&info);
> +
>      do {
>          wait_secs--;
>          sleep(1);
> @@ -5007,9 +5016,11 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs)
>          if (rc < 0)
>              goto out;
>  
> +        libxl_dominfo_dispose(&info);
> +        libxl_dominfo_init(&info);
>          rc = libxl_domain_info(ctx, &info, domid);
>          if (rc < 0)
> -            return rc;
> +            goto out;
>      } while (wait_secs > 0 && (info.current_memkb + info.outstanding_memkb) > target_memkb);
>  
>      if ((info.current_memkb + info.outstanding_memkb) <= target_memkb)
> @@ -5018,6 +5029,7 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs)
>          rc = ERROR_FAIL;
>  
>  out:
> +    libxl_dominfo_dispose(&info);
>      return rc;
>  }
>  
> @@ -5435,6 +5447,8 @@ static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
>      xs_transaction_t t;
>      int i, rc = ERROR_FAIL;
>  
> +    libxl_dominfo_init(&info);
> +
>      if (libxl_domain_info(CTX, &info, domid) < 0) {
>          LOGE(ERROR, "getting domain info list");
>          goto out;
> @@ -5454,6 +5468,7 @@ retry_transaction:
>      } else
>          rc = 0;
>  out:
> +    libxl_dominfo_dispose(&info);
>      return rc;
>  }
>  
> @@ -5463,8 +5478,11 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
>      libxl_dominfo info;
>      int i;
>  
> +    libxl_dominfo_init(&info);
> +
>      if (libxl_domain_info(CTX, &info, domid) < 0) {
>          LOGE(ERROR, "getting domain info list");
> +        libxl_dominfo_dispose(&info);
>          return ERROR_FAIL;
>      }
>      for (i = 0; i <= info.vcpu_max_id; i++) {
> @@ -5477,6 +5495,7 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
>              libxl__qmp_cpu_add(gc, domid, i);
>          }
>      }
> +    libxl_dominfo_dispose(&info);
>      return 0;
>  }
>  
> @@ -6569,12 +6588,15 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>      /* Domain UUID */
>      {
>          libxl_dominfo info;
> +        libxl_dominfo_init(&info);
>          rc = libxl_domain_info(ctx, &info, domid);
>          if (rc) {
>              LOG(ERROR, "fail to get domain info for domain %d", domid);
> +            libxl_dominfo_dispose(&info);
>              goto out;
>          }
>          libxl_uuid_copy(ctx, &d_config->c_info.uuid, &info.uuid);
> +        libxl_dominfo_dispose(&info);
>      }
>  
>      /* Memory limits:
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 74ea84b..378b620 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -2043,19 +2043,25 @@ const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid,
>                                   const char *wh)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
> -    char *uuid_string;
> +    char *uuid_string, *path;
>      libxl_dominfo info;
>      int rc;
>  
> +    libxl_dominfo_init(&info);
> +
>      rc = libxl_domain_info(ctx, &info, domid);
>      if (rc) {
>          LOGE(ERROR, "unable to find domain info for domain %"PRIu32, domid);
> -        return NULL;
> +        path = NULL;
> +        goto out;
>      }
>      uuid_string = GCSPRINTF(LIBXL_UUID_FMT, LIBXL_UUID_BYTES(info.uuid));
> -
> -    return GCSPRINTF("/var/lib/xen/userdata-%s.%u.%s.%s",
> +    path = GCSPRINTF("/var/lib/xen/userdata-%s.%u.%s.%s",
>                       wh, domid, uuid_string, userdata_userid);
> +
> + out:
> +    libxl_dominfo_dispose(&info);
> +    return path;
>  }
>  
>  static int userdata_delete(libxl__gc *gc, const char *path)
> -- 
> 1.7.10.4
>