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
[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
[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
[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