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