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

[Richard W.M. Jones]

Eric, what do you think of Pavel's analysis and/or suggested fix
below?  It seems all too plausible to me unfortunately :-(

On Fri, May 05, 2017 at 03:46:45PM +0300, Pavel Butsykin wrote:
> 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 at virtuozzo.com>
> 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 at virtuozzo.com>
> ---
>  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
> 

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/