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

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@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@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/

On 05/05/2017 12:07 PM, Richard W.M. Jones wrote:
> Eric, what do you think of Pavel's analysis and/or suggested fix
> below?  It seems all too plausible to me unfortunately :-(
> 
>> There NULL is macros which can be defined as 0 or (void*)0, again in
>> accordance with c99:
C99 permits:
#define NULL 0

but POSIX does not.  POSIX _requires_

#define NULL ((void*)0)

or the equivalent, so that NULL is properly typed as a pointer to void
in ALL cases (rather than the weaker C99 solution of letting the integer
zero constant leak), in part _because_ of the commonality of passing
NULL through varargs functions.

>>
>> 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).
Not worth worrying about in practice.  All modern libc define NULL
according to POSIX rules, rather than the weaker C99 rules.  And gnulib
guarantees the stronger definition of NULL.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 05/05/2017 12:18 PM, Eric Blake wrote:
> On 05/05/2017 12:07 PM, Richard W.M. Jones wrote:
>> Eric, what do you think of Pavel's analysis and/or suggested fix
>> below?  It seems all too plausible to me unfortunately :-(
>>
> 
>>> There NULL is macros which can be defined as 0 or (void*)0, again
in
>>> accordance with c99:
> 
> C99 permits:
> #define NULL 0
> 
> but POSIX does not.  POSIX _requires_
> 
> #define NULL ((void*)0)
> 
> or the equivalent, so that NULL is properly typed as a pointer to void
> in ALL cases (rather than the weaker C99 solution of letting the integer
> zero constant leak), in part _because_ of the commonality of passing
> NULL through varargs functions.
I forgot to cite my reference:

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stddef.h.html#tag_13_47

"NULL
    Null pointer constant. [CX] [Option Start]  The macro shall expand
to an integer constant expression with the value 0 cast to type void *.
[Option End]"
...
"RATIONALE

    The ISO C standard does not require the NULL macro to include the
cast to type void * and specifies that the NULL macro be
implementation-defined. POSIX.1-2008 requires the cast and therefore
need not be implementation-defined."

where the CX option represents requirements that POSIX makes
above-and-beyond bare C99.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org