Libguestfs - Sep 2009 - [PATCH] generator.ml: Fix string list memory leak

Parsed string lists are allocated by malloc, but were never freed.
---
 src/generator.ml |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/src/generator.ml b/src/generator.ml
index 7571f95..c72c329 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -6320,7 +6320,7 @@ and generate_fish_cmds ()          | OptString n
         | FileIn n
         | FileOut n -> pr "  const char *%s;\n" n
-        | StringList n | DeviceList n -> pr "  char *const *%s;\n"
n
+        | StringList n | DeviceList n -> pr "  char **%s;\n" n
         | Bool n -> pr "  int %s;\n" n
         | Int n -> pr "  int %s;\n" n
       ) (snd style);
@@ -6364,6 +6364,20 @@ and generate_fish_cmds ()        generate_c_call_args
~handle:"g" style;
       pr ";\n";
 
+      (* XXX: There must be a cleaner way to write this *)
+      iteri (
+        fun i ->
+            function
+            | StringList name | DeviceList name ->
+              pr "  char **%s_i = %s;\n" name name;
+              pr "  while(*%s_i) {\n" name;
+              pr "    free(*%s_i);\n" name;
+              pr "    %s_i++;\n" name;
+              pr "  }\n";
+              pr "  free(%s);\n" name;
+            | _ -> ();
+      ) (snd style);
+
       (* Check return value for errors and display command results. *)
       (match fst style with
        | RErr -> pr "  return r;\n"
-- 
1.6.2.5

On Fri, Sep 11, 2009 at 10:56:57AM +0100, Matthew Booth
wrote:
> Parsed string lists are allocated by malloc, but were never freed.
> +      (* XXX: There must be a cleaner way to write this *)
> +      iteri (
> +        fun i ->
> +            function
> +            | StringList name | DeviceList name ->
> +              pr "  char **%s_i = %s;\n" name name;
In guestfish there is a function free_strings which does what you
want, so just call:

  free_strings (%s);

instead of the loop.
> +            | _ -> ();
Using '_' to match anything is slightly dangerous.  It means that if
we added any more list-type cases to the type in the future
[eg. PathList], then the OCaml compiler wouldn't give us a warning
about not having implemented them in this code, which could lead to
misgenerated code.

The alternative is to just list all the other cases:

| String _ | Bool _ | (*etc*) -> ()

Looks fine otherwise.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html

On Fri, Sep 11, 2009 at 10:56:57AM +0100, Matthew Booth
wrote:
> +      (* XXX: There must be a cleaner way to write this *)
> +      iteri (
> +        fun i ->
> +            function
[...]
> +      ) (snd style);
Another thing - strange use of iteri here.

Just use List.iter, as in:

List.iter (
  function
  | ...
) (snd style);

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html