[Libguestfs] [PATCH] daemon: fix memory allocation and leaks in OCaml stubs

[Richard W.M. Jones]

On Thu, May 03, 2018 at 06:34:53PM +0200, Pino Toscano wrote:
> Use the cleanup handlers to free the structs (and list of structs) in
> case their OCaml->C transformation fails for any reason; use calloc()
> to not try to use uninitialized memory.
> 
> In case of lists, avoid allocating the memory for the array if there
> are no elements, since the returned pointer in that case is either NULL,
> or a free()-only pointer; also, set the list size only after the array
> is allocated, to not confuse the XDR routines.
> ---
>  generator/daemon.ml | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/generator/daemon.ml b/generator/daemon.ml
> index 559ed6898..265f0a475 100644
> --- a/generator/daemon.ml
> +++ b/generator/daemon.ml
> @@ -605,16 +605,18 @@ let generate_daemon_caml_stubs () =
>    (* Implement code for returning structs and struct lists. *)
>    let emit_return_struct typ =
>      let struc = Structs.lookup_struct typ in
> +    let uc_typ = String.uppercase_ascii typ in
>      pr "/* Implement RStruct (%S, _). */\n" typ;
>      pr "static guestfs_int_%s *\n" typ;
>      pr "return_%s (value retv)\n" typ;
>      pr "{\n";
> -    pr "  guestfs_int_%s *ret;\n" typ;
> +    pr "  CLEANUP_FREE_%s guestfs_int_%s *ret = NULL;\n" uc_typ typ;
> +    pr "  guestfs_int_%s *real_ret;\n" typ;
>      pr "  value v;\n";
>      pr "\n";
> -    pr "  ret = malloc (sizeof (*ret));\n";
> +    pr "  ret = calloc (1, sizeof (*ret));\n";
>      pr "  if (ret == NULL) {\n";
> -    pr "    reply_with_perror (\"malloc\");\n";
> +    pr "    reply_with_perror (\"calloc\");\n";
>      pr "    return NULL;\n";
>      pr "  }\n";
>      pr "\n";
> @@ -644,16 +646,20 @@ let generate_daemon_caml_stubs () =
>             pr "  ret->%s = Int_val (v);\n" n
>      ) struc.s_cols;
>      pr "\n";
> -    pr "  return ret;\n";
> +    pr "  real_ret = ret;\n";
> +    pr "  ret = NULL;\n";
> +    pr "  return real_ret;\n";
>      pr "}\n";
>      pr "\n"
>  
>    and emit_return_struct_list typ =
> +    let uc_typ = String.uppercase_ascii typ in
>      pr "/* Implement RStructList (%S, _). */\n" typ;
>      pr "static guestfs_int_%s_list *\n" typ;
>      pr "return_%s_list (value retv)\n" typ;
>      pr "{\n";
> -    pr "  guestfs_int_%s_list *ret;\n" typ;
> +    pr "  CLEANUP_FREE_%s_LIST guestfs_int_%s_list *ret = NULL;\n" uc_typ typ;
> +    pr "  guestfs_int_%s_list *real_ret;\n" typ;
>      pr "  guestfs_int_%s *r;\n" typ;
>      pr "  size_t i, len;\n";
>      pr "  value v, rv;\n";
> @@ -666,32 +672,35 @@ let generate_daemon_caml_stubs () =
>      pr "    rv = Field (rv, 1);\n";
>      pr "  }\n";
>      pr "\n";
> -    pr "  ret = malloc (sizeof *ret);\n";
> +    pr "  ret = calloc (1, sizeof *ret);\n";
>      pr "  if (ret == NULL) {\n";
> -    pr "    reply_with_perror (\"malloc\");\n";
> -    pr "    return NULL;\n";
> -    pr "  }\n";
> -    pr "  ret->guestfs_int_%s_list_len = len;\n" typ;
> -    pr "  ret->guestfs_int_%s_list_val =\n" typ;
> -    pr "    calloc (len, sizeof (guestfs_int_%s));\n" typ;
> -    pr "  if (ret->guestfs_int_%s_list_val == NULL) {\n" typ;
>      pr "    reply_with_perror (\"calloc\");\n";
> -    pr "    free (ret);\n";
>      pr "    return NULL;\n";
>      pr "  }\n";
> +    pr "  if (len > 0) {\n";
> +    pr "    ret->guestfs_int_%s_list_val =\n" typ;
> +    pr "      calloc (len, sizeof (guestfs_int_%s));\n" typ;
> +    pr "    if (ret->guestfs_int_%s_list_val == NULL) {\n" typ;
> +    pr "      reply_with_perror (\"calloc\");\n";
> +    pr "      return NULL;\n";
> +    pr "    }\n";
> +    pr "    ret->guestfs_int_%s_list_len = len;\n" typ;
> +    pr "  }\n";
>      pr "\n";
>      pr "  rv = retv;\n";
>      pr "  for (i = 0; i < len; ++i) {\n";
>      pr "    v = Field (rv, 0);\n";
>      pr "    r = return_%s (v);\n" typ;
>      pr "    if (r == NULL)\n";
> -    pr "      return NULL; /* XXX leaks memory along this error path */\n";
> +    pr "      return NULL;\n";
>      pr "    memcpy (&ret->guestfs_int_%s_list_val[i], r, sizeof (*r));\n" typ;
>      pr "    free (r);\n";
>      pr "    rv = Field (rv, 1);\n";
>      pr "  }\n";
>      pr "\n";
> -    pr "  return ret;\n";
> +    pr "  real_ret = ret;\n";
> +    pr "  ret = NULL;\n";
> +    pr "  return real_ret;\n";
>      pr "}\n";
>      pr "\n";
>    in

Looks good to me so ACK.  May be worth double checking this by running:

  make -C tests/daemon check-valgrind

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html