[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
participants (2)
-
Peter Wu -
Richard W.M. Jones