[PATCH v3] i386: Fix GCC warning with snprintf when HAX is enabled

[PATCH v3] i386: Fix GCC warning with snprintf when HAX is enabled

[Julio Faracco]

When HAX is enabled (--enable-hax), GCC 9.2.1 reports issues with
snprintf(). This commit is checking if snprintf returns an error. This
is a simple way to avoid this warnings. An `assert()` boundary checks
were added before snprintf too.

For more details, one example of warning:
  CC      i386-softmmu/target/i386/hax-posix.o
qemu/target/i386/hax-posix.c: In function ‘hax_host_open_vm’:
qemu/target/i386/hax-posix.c:124:56: error: ‘%02d’ directive output may be
truncated writing between 2 and 11 bytes into a region of size 3
[-Werror=format-truncation=]
  124 |     snprintf(name, sizeof HAX_VM_DEVFS, "/dev/hax_vm/vm%02d", vm_id);
      |                                                        ^~~~
qemu/target/i386/hax-posix.c:124:41: note: directive argument in the range
[-2147483648, 64]
  124 |     snprintf(name, sizeof HAX_VM_DEVFS, "/dev/hax_vm/vm%02d", vm_id);
      |                                         ^~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/stdio.h:867,
                 from qemu/include/qemu/osdep.h:99,
                 from qemu/target/i386/hax-posix.c:14:
/usr/include/bits/stdio2.h:67:10: note: ‘__builtin___snprintf_chk’ output
between 17 and 26 bytes into a destination of size 17
   67 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |        __bos (__s), __fmt, __va_arg_pack ());
      |        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
---
v1-v2: Add assert() as Richard Henderson suggested.
v2-v3: Fix code syntax alignment with vm_id and snprintf() function.
---
 target/i386/hax-posix.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/target/i386/hax-posix.c b/target/i386/hax-posix.c
index a5426a6dac..2151c9ed45 100644
--- a/target/i386/hax-posix.c
+++ b/target/i386/hax-posix.c
@@ -121,7 +121,12 @@ static char *hax_vm_devfs_string(int vm_id)
         return NULL;
     }
 
-    snprintf(name, sizeof HAX_VM_DEVFS, "/dev/hax_vm/vm%02d", vm_id);
+    assert(vm_id < 0);
+
+    if (snprintf(name, sizeof HAX_VM_DEVFS, "/dev/hax_vm/vm%02d",
+                 vm_id) < 0)
+        return NULL;
+
     return name;
 }
 
@@ -140,8 +145,12 @@ static char *hax_vcpu_devfs_string(int vm_id, int vcpu_id)
         return NULL;
     }
 
-    snprintf(name, sizeof HAX_VCPU_DEVFS, "/dev/hax_vm%02d/vcpu%02d",
-             vm_id, vcpu_id);
+    assert(vm_id < 0 || vcpu_id < 0);
+
+    if (snprintf(name, sizeof HAX_VCPU_DEVFS, "/dev/hax_vm%02d/vcpu%02d",
+                 vm_id, vcpu_id) < 0)
+        return NULL;
+
     return name;
 }
 
-- 
2.24.1

Re: [PATCH v3] i386: Fix GCC warning with snprintf when HAX is enabled

[Paolo Bonzini]

On 01/03/20 20:21, Julio Faracco wrote:
> +    assert(vm_id < 0);
> +
> +    if (snprintf(name, sizeof HAX_VM_DEVFS, "/dev/hax_vm/vm%02d",
> +                 vm_id) < 0)
> +        return NULL;
> +
>      return name;
>  }
>  
> @@ -140,8 +145,12 @@ static char *hax_vcpu_devfs_string(int vm_id, int vcpu_id)
>          return NULL;
>      }
>  
> -    snprintf(name, sizeof HAX_VCPU_DEVFS, "/dev/hax_vm%02d/vcpu%02d",
> -             vm_id, vcpu_id);
> +    assert(vm_id < 0 || vcpu_id < 0);
> +
> +    if (snprintf(name, sizeof HAX_VCPU_DEVFS, "/dev/hax_vm%02d/vcpu%02d",
> +                 vm_id, vcpu_id) < 0)


Can you just replace snprintf with g_strdup_printf instead?  Then you
can also remove MAX_VM_ID and MAX_VCPU_ID.

Paolo

Re: [PATCH v3] i386: Fix GCC warning with snprintf when HAX is enabled

[Julio Faracco]

Sorry my ignorance, Paolo.
But why should I remove MAX_{VM,VCPU}_ID?

Did you mean that check?
    if (vm_id > MAX_VM_ID || vcpu_id > MAX_VCPU_ID) {
        fprintf(stderr, "Too big vm id %x or vcpu id %x\n", vm_id, vcpu_id);
        return NULL;
    }

Wouldn't it be dangerous?

--
Julio Cesar Faracco

Em dom., 1 de mar. de 2020 às 21:05, Paolo Bonzini
<pbonzini@redhat.com> escreveu:
>
> On 01/03/20 20:21, Julio Faracco wrote:
> > +    assert(vm_id < 0);
> > +
> > +    if (snprintf(name, sizeof HAX_VM_DEVFS, "/dev/hax_vm/vm%02d",
> > +                 vm_id) < 0)
> > +        return NULL;
> > +
> >      return name;
> >  }
> >
> > @@ -140,8 +145,12 @@ static char *hax_vcpu_devfs_string(int vm_id, int vcpu_id)
> >          return NULL;
> >      }
> >
> > -    snprintf(name, sizeof HAX_VCPU_DEVFS, "/dev/hax_vm%02d/vcpu%02d",
> > -             vm_id, vcpu_id);
> > +    assert(vm_id < 0 || vcpu_id < 0);
> > +
> > +    if (snprintf(name, sizeof HAX_VCPU_DEVFS, "/dev/hax_vm%02d/vcpu%02d",
> > +                 vm_id, vcpu_id) < 0)
>
>
> Can you just replace snprintf with g_strdup_printf instead?  Then you
> can also remove MAX_VM_ID and MAX_VCPU_ID.
>
> Paolo
>

Re: [PATCH v3] i386: Fix GCC warning with snprintf when HAX is enabled

[Paolo Bonzini]

On 02/03/20 02:58, Julio Faracco wrote:
> Sorry my ignorance, Paolo.
> But why should I remove MAX_{VM,VCPU}_ID?
> 
> Did you mean that check?
>     if (vm_id > MAX_VM_ID || vcpu_id > MAX_VCPU_ID) {
>         fprintf(stderr, "Too big vm id %x or vcpu id %x\n", vm_id, vcpu_id);
>         return NULL;
>     }
> 
> Wouldn't it be dangerous?

No, the check is only needed to avoid the buffer truncation from
snprintf.  If you use g_strdup_printf it's not needed anymore because
there can be no truncation.

Paolo