thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH 0/2] Fix errors found by Clang static analyzer [Aug 2014]

If this information is useful, please help other people find it:
Share via:

Peter Wu

2014-Aug-07 15:59 UTC

[Libguestfs] [PATCH 0/2] Fix errors found by Clang static analyzer

Hi,

Here is one trivial initialization fix and another patch to convert a huge macro
to an inline function. The result of the expansion would show up in an assertion
which triggered a -Woverlength-strings warning.

Peter Wu (2):
  Fix garbage return value on error
  Fix overly long assertion string

 lib/hivex-internal.h | 28 ++++++++++++++++------------
 lib/node.c           | 18 +++++++++---------
 lib/utf16.c          |  2 +-
 lib/value.c          | 14 +++++++-------
 lib/write.c          | 22 +++++++++++-----------
 5 files changed, 44 insertions(+), 40 deletions(-)

-- 
2.0.2

Peter Wu

2014-Aug-07 15:59 UTC

head link

[Libguestfs] [PATCH 1/2] Fix garbage return value on error

If str contains invalid characters, ret is not initialised. Caught by
Clang static analyzer.
---
 lib/utf16.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/utf16.c b/lib/utf16.c
index f4299f2..7d275fd 100644
--- a/lib/utf16.c
+++ b/lib/utf16.c
@@ -128,7 +128,7 @@ size_t
 _hivex_utf8_strlen (const char* str, size_t len, int utf16)
 {
   const char *encoding = utf16 ? "UTF-16LE" : "LATIN1";
-  size_t ret;
+  size_t ret = 0;
   char *buf = _hivex_recode(encoding, str, len, "UTF-8", &ret);
   free(buf);
   return ret;
-- 
2.0.2

Peter Wu

2014-Aug-07 15:59 UTC

head link

[Libguestfs] [PATCH 2/2] Fix overly long assertion string

With GCC 4.9.1 I get a -Woverlength-strings warning:
hivex-internal.h:123:23: error: string length ‘3823’ is greater than ...
   (STREQLEN (((struct ntreg_hbin_block *)((char *) (h)->addr +
(offs)))->id, (eqid), 2))

The compile command can be shortened to:

    cd lib && cpp -DHAVE_CONFIG_H -I.. -I../gnulib/lib  -I. \
        -Woverlength-strings -Werror -g -O2 write.c

After that, be amazed at the huge assertion string produced by the
macro which is also visible in libhivex.so (two times).

To fix that mess, use an inline function. The `STR*` macros were moved
and a string.h header added since STREQLEN is used in the function.
---
 lib/hivex-internal.h | 28 ++++++++++++++++------------
 lib/node.c           | 18 +++++++++---------
 lib/value.c          | 14 +++++++-------
 lib/write.c          | 22 +++++++++++-----------
 4 files changed, 43 insertions(+), 39 deletions(-)

diff --git a/lib/hivex-internal.h b/lib/hivex-internal.h
index bfd24c8..e59084d 100644
--- a/lib/hivex-internal.h
+++ b/lib/hivex-internal.h
@@ -21,9 +21,20 @@
 
 #include <stdarg.h>
 #include <stddef.h>
+#include <string.h>
 
 #include "byte_conversions.h"
 
+#define STREQ(a,b) (strcmp((a),(b)) == 0)
+#define STRCASEEQ(a,b) (strcasecmp((a),(b)) == 0)
+#define STRNEQ(a,b) (strcmp((a),(b)) != 0)
+#define STRCASENEQ(a,b) (strcasecmp((a),(b)) != 0)
+#define STREQLEN(a,b,n) (strncmp((a),(b),(n)) == 0)
+#define STRCASEEQLEN(a,b,n) (strncasecmp((a),(b),(n)) == 0)
+#define STRNEQLEN(a,b,n) (strncmp((a),(b),(n)) != 0)
+#define STRCASENEQLEN(a,b,n) (strncasecmp((a),(b),(n)) != 0)
+#define STRPREFIX(a,b) (strncmp((a),(b),strlen((b))) == 0)
+
 struct hive_h {
   char *filename;
   int fd;
@@ -119,8 +130,11 @@ struct ntreg_hbin_block {
   /* Block data follows here. */
 } __attribute__((__packed__));
 
-#define BLOCK_ID_EQ(h,offs,eqid) \
-  (STREQLEN (((struct ntreg_hbin_block *)((char *) (h)->addr +
(offs)))->id, (eqid), 2))
+static inline int
+block_id_eq (const hive_h *h, size_t offs, const char *eqid)
+{
+  return (STREQLEN (((struct ntreg_hbin_block *)((char *) (h)->addr +
(offs)))->id, (eqid), 2));
+}
 
 struct ntreg_nk_record {
   int32_t seg_len;              /* length (always -ve because used) */
@@ -285,16 +299,6 @@ extern void _hivex_free_strings (char **argv);
 /* value.c */
 extern int _hivex_get_values (hive_h *h, hive_node_h node, hive_value_h
**values_ret, size_t **blocks_ret);
 
-#define STREQ(a,b) (strcmp((a),(b)) == 0)
-#define STRCASEEQ(a,b) (strcasecmp((a),(b)) == 0)
-#define STRNEQ(a,b) (strcmp((a),(b)) != 0)
-#define STRCASENEQ(a,b) (strcasecmp((a),(b)) != 0)
-#define STREQLEN(a,b,n) (strncmp((a),(b),(n)) == 0)
-#define STRCASEEQLEN(a,b,n) (strncasecmp((a),(b),(n)) == 0)
-#define STRNEQLEN(a,b,n) (strncmp((a),(b),(n)) != 0)
-#define STRCASENEQLEN(a,b,n) (strncasecmp((a),(b),(n)) != 0)
-#define STRPREFIX(a,b) (strncmp((a),(b),strlen((b))) == 0)
-
 #define DEBUG(lvl,fs,...)                                       \
   do {                                                          \
     if (h->msglvl >= (lvl)) {                                   \
diff --git a/lib/node.c b/lib/node.c
index 22d1861..1fb48cf 100644
--- a/lib/node.c
+++ b/lib/node.c
@@ -49,7 +49,7 @@ hivex_root (hive_h *h)
 size_t
 hivex_node_struct_length (hive_h *h, hive_node_h node)
 {
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return 0;
   }
@@ -71,7 +71,7 @@ hivex_node_struct_length (hive_h *h, hive_node_h node)
 char *
 hivex_node_name (hive_h *h, hive_node_h node)
 {
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return NULL;
   }
@@ -99,7 +99,7 @@ hivex_node_name (hive_h *h, hive_node_h node)
 size_t
 hivex_node_name_len (hive_h *h, hive_node_h node)
 {
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return 0;
   }
@@ -143,7 +143,7 @@ hivex_node_timestamp (hive_h *h, hive_node_h node)
 {
   int64_t ret;
 
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return -1;
   }
@@ -165,7 +165,7 @@ hivex_node_timestamp (hive_h *h, hive_node_h node)
 hive_security_h
 hivex_node_security (hive_h *h, hive_node_h node)
 {
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return 0;
   }
@@ -184,7 +184,7 @@ hivex_node_security (hive_h *h, hive_node_h node)
 hive_classname_h
 hivex_node_classname (hive_h *h, hive_node_h node)
 {
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return 0;
   }
@@ -283,7 +283,7 @@ _hivex_get_children (hive_h *h, hive_node_h node,
                      hive_node_h **children_ret, size_t **blocks_ret,
                      int flags)
 {
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return -1;
   }
@@ -491,7 +491,7 @@ check_child_is_nk_block (hive_h *h, hive_node_h child, int
flags)
   struct ntreg_hbin_block *block      (struct ntreg_hbin_block *) ((char *)
h->addr + child);
 
-  if (!BLOCK_ID_EQ (h, child, "nk")) {
+  if (!block_id_eq (h, child, "nk")) {
     SET_ERRNO (EFAULT, "subkey is not an 'nk' block (0x%zx, %d,
%d)",
                child, block->id[0], block->id[1]);
     return -1;
@@ -546,7 +546,7 @@ hivex_node_get_child (hive_h *h, hive_node_h node, const
char *nname)
 hive_node_h
 hivex_node_parent (hive_h *h, hive_node_h node)
 {
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return 0;
   }
diff --git a/lib/value.c b/lib/value.c
index f222b41..1c2356f 100644
--- a/lib/value.c
+++ b/lib/value.c
@@ -37,7 +37,7 @@ int
 _hivex_get_values (hive_h *h, hive_node_h node,
                    hive_value_h **values_ret, size_t **blocks_ret)
 {
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return -1;
   }
@@ -175,7 +175,7 @@ hivex_value_struct_length (hive_h *h, hive_value_h value)
 size_t
 hivex_value_key_len (hive_h *h, hive_value_h value)
 {
-  if (!IS_VALID_BLOCK (h, value) || !BLOCK_ID_EQ (h, value, "vk")) {
+  if (!IS_VALID_BLOCK (h, value) || !block_id_eq (h, value, "vk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'vk' block");
     return 0;
   }
@@ -199,7 +199,7 @@ hivex_value_key_len (hive_h *h, hive_value_h value)
 char *
 hivex_value_key (hive_h *h, hive_value_h value)
 {
-  if (!IS_VALID_BLOCK (h, value) || !BLOCK_ID_EQ (h, value, "vk")) {
+  if (!IS_VALID_BLOCK (h, value) || !block_id_eq (h, value, "vk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'vk' block");
     return 0;
   }
@@ -225,7 +225,7 @@ hivex_value_key (hive_h *h, hive_value_h value)
 int
 hivex_value_type (hive_h *h, hive_value_h value, hive_type *t, size_t *len)
 {
-  if (!IS_VALID_BLOCK (h, value) || !BLOCK_ID_EQ (h, value, "vk")) {
+  if (!IS_VALID_BLOCK (h, value) || !block_id_eq (h, value, "vk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'vk' block");
     return -1;
   }
@@ -247,7 +247,7 @@ hivex_value_type (hive_h *h, hive_value_h value, hive_type
*t, size_t *len)
 hive_value_h
 hivex_value_data_cell_offset (hive_h *h, hive_value_h value, size_t *len)
 {
-  if (!IS_VALID_BLOCK (h, value) || !BLOCK_ID_EQ (h, value, "vk")) {
+  if (!IS_VALID_BLOCK (h, value) || !block_id_eq (h, value, "vk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'vk' block");
     return 0;
   }
@@ -300,7 +300,7 @@ char *
 hivex_value_value (hive_h *h, hive_value_h value,
                    hive_type *t_rtn, size_t *len_rtn)
 {
-  if (!IS_VALID_BLOCK (h, value) || !BLOCK_ID_EQ (h, value, "vk")) {
+  if (!IS_VALID_BLOCK (h, value) || !block_id_eq (h, value, "vk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'vk' block");
     return NULL;
   }
@@ -368,7 +368,7 @@ hivex_value_value (hive_h *h, hive_value_h value,
     return ret;
   } else {
     if (!IS_VALID_BLOCK (h, data_offset) ||
-        !BLOCK_ID_EQ (h, data_offset, "db")) {
+        !block_id_eq (h, data_offset, "db")) {
       SET_ERRNO (EINVAL,
                  "declared data length is longer than the block and "
                  "block is not a db block (data 0x%zx, data len
%zu)",
diff --git a/lib/write.c b/lib/write.c
index 0b25549..9de8ecd 100644
--- a/lib/write.c
+++ b/lib/write.c
@@ -340,7 +340,7 @@ insert_lf_record (hive_h *h, size_t old_offs, size_t posn,
   /* Work around C stupidity.
    * http://www.redhat.com/archives/libguestfs/2010-February/msg00056.html
    */
-  int test = BLOCK_ID_EQ (h, old_offs, "lf") || BLOCK_ID_EQ (h,
old_offs, "lh");
+  int test = block_id_eq (h, old_offs, "lf") || block_id_eq (h,
old_offs, "lh");
   assert (test);
 
   struct ntreg_lf_record *old_lf @@ -399,7 +399,7 @@ insert_li_record (hive_h
*h, size_t old_offs, size_t posn,
                   const char *name, hive_node_h node)
 {
   assert (IS_VALID_BLOCK (h, old_offs));
-  assert (BLOCK_ID_EQ (h, old_offs, "li"));
+  assert (block_id_eq (h, old_offs, "li"));
 
   struct ntreg_ri_record *old_li      (struct ntreg_ri_record *) ((char *)
h->addr + old_offs);
@@ -452,7 +452,7 @@ static int
 compare_name_with_nk_name (hive_h *h, const char *name, hive_node_h nk_offs)
 {
   assert (IS_VALID_BLOCK (h, nk_offs));
-  assert (BLOCK_ID_EQ (h, nk_offs, "nk"));
+  assert (block_id_eq (h, nk_offs, "nk"));
 
   /* Name in nk is not necessarily nul-terminated. */
   char *nname = hivex_node_name (h, nk_offs);
@@ -493,7 +493,7 @@ insert_subkey (hive_h *h, const char *name,
    * the end.
    */
   for (i = 0; blocks[i] != 0; ++i) {
-    if (BLOCK_ID_EQ (h, blocks[i], "lf") || BLOCK_ID_EQ (h,
blocks[i], "lh")) {
+    if (block_id_eq (h, blocks[i], "lf") || block_id_eq (h,
blocks[i], "lh")) {
       old_offs = blocks[i];
       old_lf = (struct ntreg_lf_record *) ((char *) h->addr + old_offs);
       old_li = NULL;
@@ -504,7 +504,7 @@ insert_subkey (hive_h *h, const char *name,
           goto insert_it;
       }
     }
-    else if (BLOCK_ID_EQ (h, blocks[i], "li")) {
+    else if (block_id_eq (h, blocks[i], "li")) {
       old_offs = blocks[i];
       old_lf = NULL;
       old_li = (struct ntreg_ri_record *) ((char *) h->addr + old_offs);
@@ -565,7 +565,7 @@ insert_subkey (hive_h *h, const char *name,
   }
   else {
     for (i = 0; blocks[i] != 0; ++i) {
-      if (BLOCK_ID_EQ (h, blocks[i], "ri")) {
+      if (block_id_eq (h, blocks[i], "ri")) {
         struct ntreg_ri_record *ri            (struct ntreg_ri_record *) ((char
*) h->addr + blocks[i]);
         for (j = 0; j < le16toh (ri->nr_offsets); ++j)
@@ -593,7 +593,7 @@ hivex_node_add_child (hive_h *h, hive_node_h parent, const
char *name)
 {
   CHECK_WRITABLE (0);
 
-  if (!IS_VALID_BLOCK (h, parent) || !BLOCK_ID_EQ (h, parent, "nk"))
{
+  if (!IS_VALID_BLOCK (h, parent) || !block_id_eq (h, parent, "nk"))
{
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return 0;
   }
@@ -649,7 +649,7 @@ hivex_node_add_child (hive_h *h, hive_node_h parent, const
char *name)
   size_t parent_sk_offset = le32toh (parent_nk->sk);
   parent_sk_offset += 0x1000;
   if (!IS_VALID_BLOCK (h, parent_sk_offset) ||
-      !BLOCK_ID_EQ (h, parent_sk_offset, "sk")) {
+      !block_id_eq (h, parent_sk_offset, "sk")) {
     SET_ERRNO (EFAULT,
                "parent sk is not a valid block (%zu)",
parent_sk_offset);
     return 0;
@@ -747,7 +747,7 @@ hivex_node_add_child (hive_h *h, hive_node_h parent, const
char *name)
 static int
 delete_sk (hive_h *h, size_t sk_offset)
 {
-  if (!IS_VALID_BLOCK (h, sk_offset) || !BLOCK_ID_EQ (h, sk_offset,
"sk")) {
+  if (!IS_VALID_BLOCK (h, sk_offset) || !block_id_eq (h, sk_offset,
"sk")) {
     SET_ERRNO (EFAULT, "not an sk record: 0x%zx", sk_offset);
     return -1;
   }
@@ -852,7 +852,7 @@ hivex_node_delete_child (hive_h *h, hive_node_h node)
 {
   CHECK_WRITABLE (-1);
 
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return -1;
   }
@@ -925,7 +925,7 @@ hivex_node_set_values (hive_h *h, hive_node_h node,
 {
   CHECK_WRITABLE (-1);
 
-  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
+  if (!IS_VALID_BLOCK (h, node) || !block_id_eq (h, node, "nk")) {
     SET_ERRNO (EINVAL, "invalid block or not an 'nk' block");
     return -1;
   }
-- 
2.0.2

Richard W.M. Jones

2014-Aug-07 16:12 UTC

head link

Re: [Libguestfs] [PATCH 0/2] Fix errors found by Clang static analyzer

On Thu, Aug 07, 2014 at 05:59:48PM +0200, Peter Wu
wrote:> Hi,
> 
> Here is one trivial initialization fix and another patch to convert a huge
macro
> to an inline function. The result of the expansion would show up in an
assertion
> which triggered a -Woverlength-strings warning.
Thanks -- I have pushed both of these patches upstream.

Did you come to a final conclusion about the Python binding problem?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Peter Wu

2014-Aug-07 16:16 UTC

head link

Re: [Libguestfs] [PATCH 0/2] Fix errors found by Clang static analyzer

Hi Richard,

Thanks for pushing. I have not yet had the time to continue on that, might have
a look at it in the night. These patches were sitting in my tree so I thought it
would be a good idea to send it early.

Kind regards,
Peter
https://lekensteyn.nl
(pardon my brevity, top-posting and formatting, sent from my phone)


On August 7, 2014 6:12:55 PM CEST, "Richard W.M. Jones"
<rjones@redhat.com> wrote:>On Thu, Aug 07, 2014 at 05:59:48PM +0200, Peter Wu wrote:
>> Hi,
>> 
>> Here is one trivial initialization fix and another patch to convert a
>huge macro
>> to an inline function. The result of the expansion would show up in
>an assertion
>> which triggered a -Woverlength-strings warning.
>
>Thanks -- I have pushed both of these patches upstream.
>
>Did you come to a final conclusion about the Python binding problem?
>
>Rich.

Libguestfs - Aug 2014 - Re: [PATCH 0/2] Fix errors found by Clang static analyzer

[Libguestfs] [PATCH 0/2] Fix errors found by Clang static analyzer

[Libguestfs] [PATCH 1/2] Fix garbage return value on error

[Libguestfs] [PATCH 2/2] Fix overly long assertion string

Re: [Libguestfs] [PATCH 0/2] Fix errors found by Clang static analyzer

Re: [Libguestfs] [PATCH 0/2] Fix errors found by Clang static analyzer