Re: [Libguestfs] [PATCH v2 1/2] appliance: search all types of appliances for each path separately

On 05.05.2017 12:27, Richard W.M. Jones wrote: > >Looks good. I'll push this if it passes 'make check && make check-valgrind' >which I'm currently running. Thanks. It is not connected with the current patch-set, but I noticed a possible bug in appliance.c: static int dir_contains_files (guestfs_h *g, const char *dir, ...) { va_list args; const char *file; va_start (args, dir); while ((file = va_arg (args, const char *)) != NULL) { if (!dir_contains_file (g, dir, file)) { ... static int contains_fixed_appliance (guestfs_h *g, const char *path, void *data) { return dir_contains_files (g, path, "README.fixed", "kernel", "initrd", "root", NULL); } Passing NULL as the last argument to dir_contains_file() can leads to undefined behavior in accordance with C99. 7.15.1.1 If there is no actual next argument, or if type is not compatible with the type of the actual next argument (as promoted according to the default argument promotions), the behavior is undefined, except for the following cases: — one type is a signed integer type, the other type is the corresponding unsigned integer type, and the value is representable in both types; — one type is pointer to void and the other is a pointer to a character type There NULL is macros which can be defined as 0 or (void*)0, again in accordance with c99: 7.17 Common definitions .. The macros are NULL which expands to an implementation-defined null pointer constant; 6.3.2.3 Pointers ... 3 An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant. If a null pointer constant is converted to a pointer type, the resulting pointer, called a null pointer, is guaranteed to compare unequal to a pointer to any object or function. In practice, this means that if NULL is defined as integer and we have 64 bit architecture, then here as the last argument - 4 bytes are written on the stack: dir_contains_files (g, path, "README.fixed", "kernel", "initrd", "root", NULL); But va_arg() will read 8 bytes: while ((file = va_arg (args, const char *)) != NULL) { I don't know how real can be the conditions to reproduce this, I mean the standard header where #define NULL (0), but as an extra precaution I can offer a patch (attached). >Rich. >

>From 44164bb3ac96ecf92892bcb25aba85cf38f5769f Mon Sep 17 00:00:00 2001 From: Pavel Butsykin <pbutsykin(a)virtuozzo.com&gt; Date: Fri, 5 May 2017 15:38:19 +0300 Subject: [PATCH] applaince: iterable args should be compatible with type used by va_arg() Signed-off-by: Pavel Butsykin <pbutsykin(a)virtuozzo.com&gt; --- lib/appliance.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/appliance.c b/lib/appliance.c index 66f91cb0a..42591e7d7 100644 --- a/lib/appliance.c +++ b/lib/appliance.c @@ -222,7 +222,7 @@ search_appliance (guestfs_h *g, struct appliance_files *appliance) static int contains_old_style_appliance (guestfs_h *g, const char *path, void *data) { - return dir_contains_files (g, path, kernel_name, initrd_name, NULL); + return dir_contains_files (g, path, kernel_name, initrd_name, (void *) NULL); } static int @@ -230,7 +230,7 @@ contains_fixed_appliance (guestfs_h *g, const char *path, void *data) { return dir_contains_files (g, path, "README.fixed", - "kernel", "initrd", "root", NULL); + "kernel", "initrd", "root", (void *) NULL); } static int @@ -238,7 +238,7 @@ contains_supermin_appliance (guestfs_h *g, const char *path, void *data) { return dir_contains_files (g, path, "supermin.d/base.tar.gz", - "supermin.d/packages", NULL); + "supermin.d/packages", (void *) NULL); } /** -- 2.11.0