Libguestfs - Aug 2014 - [PATCH 0/2] Fix (minor) bugs found by Clang static analyzer

Hi,

Here are more fixes for bugs caught my Clang static analyzer. Two bugs are
remaining, namely a memleak in the perl bindings. I am not an expert in that
area and did not manage to fix it in a correct way, so I will just describe it.

The ASetValues and ASetValue functions call a function which allocates memory.
The generated code puts this before PREINIT. Then there is the typemap for
hive_h object. This code can return if the validation fails which causes a
memleak.

So, could some Perl programmer pick this up?

Peter Wu (2):
  Silence dead assigmnents/initialization/increments
  Avoid calling calloc(0, x)

 lib/write.c  | 12 +++++-------
 sh/hivexsh.c |  4 ++++
 2 files changed, 9 insertions(+), 7 deletions(-)

-- 
2.0.2

PS. now I'll have a look at the Python bug wrt set_value.

Some of these assignments were introduced in commit
c9d5cd059c45fd3aa6d16b3ba185d7cb3a08de9e ("hivex: Fix allocations that
may move C heap buffer."), but turn out to be unnecessary (unused).
Replace the assignments by a comment in case someone extends the code
later.

Caught by Clang static analyzer.
---
 lib/write.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/lib/write.c b/lib/write.c
index 9de8ecd..abd12c5 100644
--- a/lib/write.c
+++ b/lib/write.c
@@ -264,7 +264,6 @@ delete_values (hive_h *h, hive_node_h node)
     int is_inline;
     len = le32toh (vk->data_len);
     is_inline = !!(len & 0x80000000); /* top bit indicates is inline */
-    len &= 0x7fffffff;
 
     if (!is_inline) {           /* non-inline, so remove data block */
       size_t data_offset = le32toh (vk->data_offset);
@@ -707,7 +706,7 @@ hivex_node_add_child (hive_h *h, hive_node_h parent, const
char *name)
     /* Recalculate pointers that could have been invalidated by
      * previous call to allocate_block (via new_lh_record).
      */
-    nk = (struct ntreg_nk_record *) ((char *) h->addr + nkoffset);
+    /* nk could be invalid here */
     parent_nk = (struct ntreg_nk_record *) ((char *) h->addr + parent);
 
     DEBUG (2, "no keys, allocated new lh-record at 0x%zx", lh_offs);
@@ -723,7 +722,7 @@ hivex_node_add_child (hive_h *h, hive_node_h parent, const
char *name)
     /* Recalculate pointers that could have been invalidated by
      * previous call to allocate_block (via new_lh_record).
      */
-    nk = (struct ntreg_nk_record *) ((char *) h->addr + nkoffset);
+    /* nk could be invalid here */
     parent_nk = (struct ntreg_nk_record *) ((char *) h->addr + parent);
   }
 
@@ -950,8 +949,6 @@ hivex_node_set_values (hive_h *h, hive_node_h node,
   nk->nr_values = htole32 (nr_values);
   nk->vallist = htole32 (vallist_offs - 0x1000);
 
-  struct ntreg_value_list *vallist -    (struct ntreg_value_list *) ((char *)
h->addr + vallist_offs);
 
   size_t i;
   for (i = 0; i < nr_values; ++i) {
@@ -970,7 +967,8 @@ hivex_node_set_values (hive_h *h, hive_node_h node,
      * previous call to allocate_block.
      */
     nk = (struct ntreg_nk_record *) ((char *) h->addr + node);
-    vallist = (struct ntreg_value_list *) ((char *) h->addr + vallist_offs);
+    struct ntreg_value_list *vallist +      (struct ntreg_value_list *) ((char
*) h->addr + vallist_offs);
 
     vallist->offset[i] = htole32 (vk_offs - 0x1000);
 
@@ -1000,7 +998,7 @@ hivex_node_set_values (hive_h *h, hive_node_h node,
        * previous call to allocate_block.
        */
       nk = (struct ntreg_nk_record *) ((char *) h->addr + node);
-      vallist = (struct ntreg_value_list *) ((char *) h->addr +
vallist_offs);
+      /* vallist could be invalid here */
       vk = (struct ntreg_vk_record *) ((char *) h->addr + vk_offs);
 
       memcpy ((char *) h->addr + offs + 4, values[i].value, values[i].len);
-- 
2.0.2

The return value for zero-size allocations is implementation-defined
(can be NULL or a unique pointer for free()). Just return early here as
there is nothing to print.

Caught by Clang static analyzer.
---
 sh/hivexsh.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sh/hivexsh.c b/sh/hivexsh.c
index f578ccc..243682c 100644
--- a/sh/hivexsh.c
+++ b/sh/hivexsh.c
@@ -622,6 +622,10 @@ cmd_ls (char *args)
   for (len = 0; children[len] != 0; ++len)
     ;
 
+  /* No children, no need to print anything. */
+  if (len == 0)
+      return 0;
+
   char **names = calloc (len, sizeof (char *));
   if (names == NULL) {
     perror ("malloc");
-- 
2.0.2

On Fri, Aug 08, 2014 at 11:31:07AM +0200, Peter Wu
wrote:
> Hi,
>
> Here are more fixes for bugs caught my Clang static analyzer. Two
> bugs are remaining, namely a memleak in the perl bindings. I am not
> an expert in that area and did not manage to fix it in a correct
> way, so I will just describe it.
Thanks - pushed both.
> The ASetValues and ASetValue functions call a function which
> allocates memory.  The generated code puts this before PREINIT. Then
> there is the typemap for hive_h object. This code can return if the
> validation fails which causes a memleak.
>
> So, could some Perl programmer pick this up?
I'm not super-worried by this as:

 - It's only a memory leak.

 - It doesn't seem to be exploitable.

 - It can be avoided easily by not passing a bogus handle in Perl methods.

 - Fixing it is complex and is most likely to introduce real bugs.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org