thr3ads.net - Libguestfs - [Libguestfs] [PATCH 2/3] lib/proto: suppress "may be used uninitialized" in send_file

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

Laszlo Ersek

2021-Oct-11 22:36 UTC

[Libguestfs] [PATCH 0/3] daemon/yara: fix undefined behavior due to Yara 4.0 API changes

I *almost* got around looking at RHBZ#1995391 again, but then I pulled
master to build a new baseline, and then:

- the Yara test case failed (I had recently installed the Yara devel
  package),

- it turns out the test case failure comes from a genuine bug,

- it turns out that gcc warns about the bug, but I don't notice gcc
  warnings unless I ask for "-Werror" (I don't really
"watch" the
  build),

- it turns out the "./configure --help" hint on "-Werror" is
inexact :)

Thanks,
Laszlo

Laszlo Ersek (3):
  build: fix typo in "--enable-werror" help string
  lib/proto: suppress "may be used uninitialized" in
    send_file_complete()
  daemon/yara: fix undefined behavior due to Yara 4.0 API changes

 daemon/yara.c   | 20 ++++++++++++++++----
 lib/proto.c     |  2 +-
 m4/guestfs-c.m4 |  2 +-
 3 files changed, 18 insertions(+), 6 deletions(-)

-- 
2.19.1.3.g30247aa5d201

Laszlo Ersek

2021-Oct-11 22:36 UTC

head link

[Libguestfs] [PATCH 1/3] build: fix typo in "--enable-werror" help string

While <https://libguestfs.org/guestfs-building.1.html> correctly documents
the "--enable-werror" option, the "./configure" help text
itself doesn't.
Replace "--enable-error" with "--enable-werror" now.

Fixes: 0f54df53d26e4c293871fb30bce88511e1d61d6c
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 m4/guestfs-c.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/m4/guestfs-c.m4 b/m4/guestfs-c.m4
index 08fd4e1b5067..95eddb231957 100644
--- a/m4/guestfs-c.m4
+++ b/m4/guestfs-c.m4
@@ -30,7 +30,7 @@ test "x$U" != "x" &&
AC_MSG_ERROR([Compiler not ANSI compliant])
 AM_PROG_CC_C_O
 
 AC_ARG_ENABLE([werror],
-    [AS_HELP_STRING([--enable-error],
+    [AS_HELP_STRING([--enable-werror],
                     [turn on lots of GCC warnings (for developers)])],
      [case $enableval in
       yes|no) ;;
-- 
2.19.1.3.g30247aa5d201

Laszlo Ersek

2021-Oct-11 22:36 UTC

head link

[Libguestfs] [PATCH 2/3] lib/proto: suppress "may be used uninitialized" in send_file_complete()

gcc emits the following warning:
> proto.c: In function ?send_file_complete?:
> proto.c:437:10: error: ?buf? may be used uninitialized
> [-Werror=maybe-uninitialized]
>   437 |   return send_file_chunk (g, 0, buf, 0);
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In theory, passing the 1-byte array "buf", with indeterminate
contents, to
xdr_bytes() ultimately, could be fine -- assuming xdr_bytes() never reads
the contents of the buffer, due to the buffer size being zero. However,
the xdr_bytes() manual does not seem to guarantee this (it also does not
explicitly permit passing a NULL buffer alongside size=0, which would be
even simpler for the caller).

In order to shut up the compiler, just zero-initialize the buffer --
that's simpler than adding diagnostics pragmas. The
"maybe-uninitialized"
warning is otherwise very useful, so keep it globally enabled (per
WARN_CFLAGS / WERROR_CFLAGS).

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 lib/proto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/proto.c b/lib/proto.c
index 3976e98b56d0..f798ece05e32 100644
--- a/lib/proto.c
+++ b/lib/proto.c
@@ -433,7 +433,7 @@ send_file_cancellation (guestfs_h *g)
 static int
 send_file_complete (guestfs_h *g)
 {
-  char buf[1];
+  char buf[1] = { '\0' };
   return send_file_chunk (g, 0, buf, 0);
 }
 
-- 
2.19.1.3.g30247aa5d201

Laszlo Ersek

2021-Oct-11 22:36 UTC

head link

[Libguestfs] [PATCH 3/3] daemon/yara: fix undefined behavior due to Yara 4.0 API changes

Currently, the Yara test case ("yara/test-yara-scan.sh") fails, with
the
following obscure error message:
> ><fs> yara-scan /text.txt
> libguestfs: error: deserialise_yara_detection_list:
Namely, the Yara rule match list serialization / de-serialization, between
the daemon and the library, is broken. It is caused by the following
incompatible pointer passing (i.e., undefined behavior), in function
do_internal_yara_scan(), file "daemon/yara.c":
>   r = yr_rules_scan_fd (rules, fd, 0, yara_rules_callback, (void *) path,
0);                                        ^^^^^^^^^^^^^^^^^^^

The prototype of yara_rules_callback() is:
> static int
> yara_rules_callback (int code, void *message, void *data)
however, in Yara commit 2b121b166d25 ("Track string matches using
YR_SCAN_CONTEXT.", 2020-02-27), which was included in the upstream v4.0.0
release, the rules callback prototype was changed as follows:
> diff --git a/libyara/include/yara/types.h b/libyara/include/yara/types.h
> index cad095cd70c2..f415033c4aa6 100644
> --- a/libyara/include/yara/types.h
> +++ b/libyara/include/yara/types.h
> @@ -661,6 +644,7 @@ struct YR_MEMORY_BLOCK_ITERATOR
>
>
>  typedef int (*YR_CALLBACK_FUNC)(
> +    YR_SCAN_CONTEXT* context,
>      int message,
>      void* message_data,
>      void* user_data);
Therefore, the yara_rules_callback() function is entered with a mismatched
parameter list in the daemon, and so it passes garbage to
send_detection_info(), for serializing the match list.

This incompatible change was in fact documented by the Yara project:

 
https://github.com/VirusTotal/yara/wiki/Backward-incompatible-changes-in-YARA-4.0-API#scanning-callback

Gcc too warns about the incompatible pointer type, under
"-Wincompatible-pointer-types". However, libguestfs is built without
"-Werror" by default, so the warning is easy to miss, and the bug only
manifests at runtime.

(The same problem exists for yr_compiler_set_callback() /
compile_error_callback():

 
https://github.com/VirusTotal/yara/wiki/Backward-incompatible-changes-in-YARA-4.0-API#compiler-callback

except that this instance of the problem is not triggered by the test
case, as the rule list always compiles.)

Rather than simply fixing the parameter lists, consider the following
approach.

If Yara's YR_CALLBACK_FUNC and YR_COMPILER_CALLBACK_FUNC typedefs were not
for *pointer* types but actual *function* prototypes, then we could use
them directly in the declarations of our callback functions. Then any
future changes in the param lists would force a "conflicting types"
*compilation error* (not a warning). Illustration:

  /* this is *not* a pointer type */
  typedef int HELLO_FUNC (void);

  /* function declarations */
  static HELLO_FUNC my_hello_good;
  static HELLO_FUNC my_hello_bad;

  /* function definition, with explicit parameter list */
  static int my_hello_good (void) { return 1; }

  /* function definition with wrong param list -> compilation error */
  static int my_hello_bad (int i) { return i; }

Unfortunately, given that the Yara-provided typedefs are already pointers,
we can't do this, in standard C anyway. Type derivation only allows for
array, structure, union, function, and pointer type derivation; it does
not allow "undoing" previous derivations.

However, using gcc's "typeof" keyword, the idea is possible. Given
YR_CALLBACK_FUNC, the expression

  (YR_CALLBACK_FUNC)NULL

is a well-defined null pointer, and the function designator expression

  *(YR_CALLBACK_FUNC)NULL

has the desired function type.

Of course, evaluating this expression would be undefined behavior, but in
the GCC extension expression

  typeof (*(YR_CALLBACK_FUNC)NULL)

the operand of the "typeof" operator is never evaluated, as it does
not
have a variably modified type. We can therefore use this "typeof" in
the
same role as HELLO_FUNC had in the above example.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 daemon/yara.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/daemon/yara.c b/daemon/yara.c
index 9e7bc9414957..e5f5b89eb6d9 100644
--- a/daemon/yara.c
+++ b/daemon/yara.c
@@ -56,11 +56,22 @@ static YR_RULES *rules = NULL;
 static bool initialized = false;
 
 static int compile_rules_file (const char *);
-static void compile_error_callback (int, const char *, int, const char *, void
*);
 static void cleanup_destroy_yara_compiler (void *ptr);
-static int yara_rules_callback (int , void *, void *);
 static int send_detection_info (const char *, YR_RULE *);
 
+/* Typedefs that effectively strip the pointer derivation from Yara's
+ * YR_CALLBACK_FUNC and YR_COMPILER_CALLBACK_FUNC types, using GCC's
"typeof"
+ * extension.
+ */
+typedef typeof (*(YR_CALLBACK_FUNC)NULL)          guestfs_yr_callback;
+typedef typeof (*(YR_COMPILER_CALLBACK_FUNC)NULL) guestfs_yr_compiler_callback;
+
+/* Declarations of our callback functions expressed in terms of Yara's
+ * typedefs. Note: these are *function declarations*.
+ */
+static guestfs_yr_callback          yara_rules_callback;
+static guestfs_yr_compiler_callback compile_error_callback;
+
 /* Has one FileIn parameter.
  * Takes optional arguments, consult optargs_bitmask.
  */
@@ -210,7 +221,7 @@ compile_rules_file (const char *rules_path)
  */
 static void
 compile_error_callback (int level, const char *name, int line,
-                        const char *message, void *data)
+                        const YR_RULE* rule, const char *message, void *data)
 {
   if (level == YARA_ERROR_LEVEL_ERROR)
     fprintf (stderr, "Yara error (line %d): %s\n", line, message);
@@ -222,7 +233,8 @@ compile_error_callback (int level, const char *name, int
line,
  * Return 0 on success, -1 on error.
  */
 static int
-yara_rules_callback (int code, void *message, void *data)
+yara_rules_callback (YR_SCAN_CONTEXT *context, int code, void *message,
+                     void *data)
 {
   int ret = 0;
 
-- 
2.19.1.3.g30247aa5d201

Libguestfs - Oct 2021 - [PATCH 2/3] lib/proto: suppress "may be used uninitialized" in send_file_complete()

[Libguestfs] [PATCH 0/3] daemon/yara: fix undefined behavior due to Yara 4.0 API changes

[Libguestfs] [PATCH 1/3] build: fix typo in "--enable-werror" help string

[Libguestfs] [PATCH 2/3] lib/proto: suppress "may be used uninitialized" in send_file_complete()

[Libguestfs] [PATCH 3/3] daemon/yara: fix undefined behavior due to Yara 4.0 API changes