[libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList

Michal Privoznik posted 1 patch 5 years ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/2c27c79f5b7bbc37d8a11ff863f49e6e9b237eb8.1553680165.git.mprivozn@redhat.com
src/qemu/qemu_capabilities.c | 6 +++---
src/qemu/qemu_capabilities.h | 2 +-
tests/qemuxml2argvtest.c     | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
[libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList
Posted by Michal Privoznik 5 years ago
There is one specific caller (testInfoSetArgs() in
qemuxml2argvtest.c) which expect the  va_list argument to change
after returning from the virQEMUCapsSetVAList() function.
However, since we are passing plain va_list this is not
guaranteed. The man page of stdarg(3) says:

  If ap is passed to a function that uses va_arg(ap,type), then
  the value of ap is undefined after the return of that function.

(ap is a variable of type va_list)

I've seen this in action in fact: on i686 the qemuxml2argvtest
fails on the second test case because testInfoSetArgs() sees
ARG_QEMU_CAPS and callse virQEMUCapsSetVAList to process the
capabilities (in this case there's just one
QEMU_CAPS_SECCOMP_BLACKLIST). But since the changes are not
reflected in the caller, in the next iteration testInfoSetArgs()
sees the qemu capability and not ARG_END.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

This passed successfully on x86_64 and i686 in my testing.

 src/qemu/qemu_capabilities.c | 6 +++---
 src/qemu/qemu_capabilities.h | 2 +-
 tests/qemuxml2argvtest.c     | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c5954edaf0..e182a2c2e5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1665,11 +1665,11 @@ virQEMUCapsSet(virQEMUCapsPtr qemuCaps,
 
 void
 virQEMUCapsSetVAList(virQEMUCapsPtr qemuCaps,
-                     va_list list)
+                     va_list *list)
 {
     int flag;
 
-    while ((flag = va_arg(list, int)) < QEMU_CAPS_LAST)
+    while ((flag = va_arg(*list, int)) < QEMU_CAPS_LAST)
         ignore_value(virBitmapSetBit(qemuCaps->flags, flag));
 }
 
@@ -1680,7 +1680,7 @@ virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...)
     va_list list;
 
     va_start(list, qemuCaps);
-    virQEMUCapsSetVAList(qemuCaps, list);
+    virQEMUCapsSetVAList(qemuCaps, &list);
     va_end(list);
 }
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 7625d754a3..ceab8c9154 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -519,7 +519,7 @@ void virQEMUCapsSet(virQEMUCapsPtr qemuCaps,
                     virQEMUCapsFlags flag) ATTRIBUTE_NONNULL(1);
 
 void virQEMUCapsSetVAList(virQEMUCapsPtr qemuCaps,
-                          va_list list) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+                          va_list *list) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 void virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...) ATTRIBUTE_NONNULL(1);
 
 void virQEMUCapsClear(virQEMUCapsPtr qemuCaps,
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 6e7d1b0b9a..a7a8d77e1d 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -650,7 +650,7 @@ testInfoSetArgs(struct testInfo *info,
         case ARG_QEMU_CAPS:
             if (qemuCaps || !(qemuCaps = virQEMUCapsNew()))
                 goto cleanup;
-            virQEMUCapsSetVAList(qemuCaps, argptr);
+            virQEMUCapsSetVAList(qemuCaps, &argptr);
             break;
 
         case ARG_GIC:
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList
Posted by Andrea Bolognani 5 years ago
On Wed, 2019-03-27 at 10:50 +0100, Michal Privoznik wrote:
> There is one specific caller (testInfoSetArgs() in
> qemuxml2argvtest.c) which expect the  va_list argument to change

s/  / /

> after returning from the virQEMUCapsSetVAList() function.
> However, since we are passing plain va_list this is not
> guaranteed. The man page of stdarg(3) says:
> 
>   If ap is passed to a function that uses va_arg(ap,type), then
>   the value of ap is undefined after the return of that function.
> 
> (ap is a variable of type va_list)
> 
> I've seen this in action in fact: on i686 the qemuxml2argvtest
> fails on the second test case because testInfoSetArgs() sees
> ARG_QEMU_CAPS and callse virQEMUCapsSetVAList to process the

s/callse/calls/

> capabilities (in this case there's just one
> QEMU_CAPS_SECCOMP_BLACKLIST). But since the changes are not
> reflected in the caller, in the next iteration testInfoSetArgs()
> sees the qemu capability and not ARG_END.

s/qemu/QEMU/

> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> This passed successfully on x86_64 and i686 in my testing.
> 
>  src/qemu/qemu_capabilities.c | 6 +++---
>  src/qemu/qemu_capabilities.h | 2 +-
>  tests/qemuxml2argvtest.c     | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)

The changes look okay to me, so

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

however I'd like to hear Eric's opinion before this gets merged.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList
Posted by Daniel P. Berrangé 5 years ago
On Wed, Mar 27, 2019 at 01:02:07PM +0100, Andrea Bolognani wrote:
> On Wed, 2019-03-27 at 10:50 +0100, Michal Privoznik wrote:
> > There is one specific caller (testInfoSetArgs() in
> > qemuxml2argvtest.c) which expect the  va_list argument to change
> 
> s/  / /
> 
> > after returning from the virQEMUCapsSetVAList() function.
> > However, since we are passing plain va_list this is not
> > guaranteed. The man page of stdarg(3) says:
> > 
> >   If ap is passed to a function that uses va_arg(ap,type), then
> >   the value of ap is undefined after the return of that function.
> > 
> > (ap is a variable of type va_list)
> > 
> > I've seen this in action in fact: on i686 the qemuxml2argvtest
> > fails on the second test case because testInfoSetArgs() sees
> > ARG_QEMU_CAPS and callse virQEMUCapsSetVAList to process the
> 
> s/callse/calls/
> 
> > capabilities (in this case there's just one
> > QEMU_CAPS_SECCOMP_BLACKLIST). But since the changes are not
> > reflected in the caller, in the next iteration testInfoSetArgs()
> > sees the qemu capability and not ARG_END.
> 
> s/qemu/QEMU/
> 
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> > 
> > This passed successfully on x86_64 and i686 in my testing.
> > 
> >  src/qemu/qemu_capabilities.c | 6 +++---
> >  src/qemu/qemu_capabilities.h | 2 +-
> >  tests/qemuxml2argvtest.c     | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> The changes look okay to me, so
> 
>   Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> however I'd like to hear Eric's opinion before this gets merged.

I'll trust Michal when he says it works, but it feels a bit odd to be
passing a pointer to a va_list around. Personally I would have just
inlined virQEMUCapsSetVAList() as it is only 2 lines of code.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList
Posted by Eric Blake 5 years ago
On 3/27/19 7:02 AM, Andrea Bolognani wrote:
> On Wed, 2019-03-27 at 10:50 +0100, Michal Privoznik wrote:
>> There is one specific caller (testInfoSetArgs() in
>> qemuxml2argvtest.c) which expect the  va_list argument to change
> 
> s/  / /
> 
>> after returning from the virQEMUCapsSetVAList() function.
>> However, since we are passing plain va_list this is not
>> guaranteed. The man page of stdarg(3) says:
>>
>>   If ap is passed to a function that uses va_arg(ap,type), then
>>   the value of ap is undefined after the return of that function.
>>
>> (ap is a variable of type va_list)

> however I'd like to hear Eric's opinion before this gets merged.

Passing a va_list *arg as a parameter is doable, but it comes with odd
effects. That is because the C standard permits both of the following
implementation styles:

typedef struct __something *va_list;
typedef struct __something va_list[];

but you get different semantics on what happens if you try to take the
address of a va_list parameter, (C parameter lists undergo pointer
decay, and taking the address of a parameter results in either the
address of a pointer or the address of the first member of an array -
which are different levels of dereferencing and thus different types).
The type 'pointer to va_list' is sane, and the computation of
'&local_va_list' is sane; it is only '&parameter_va_list' that gets you
in trouble.  Here's where I demonstrated it further for the qemu list:

https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg00171.html

The only portable way to take va_list from one function to pass to a
va_list * of another function is to va_copy() from the parameter into a
local va_list, then take the address of the local.  But if you are the
original owner of a local va_list (because your function took ...
instead of va_list), you don't have to worry about the hoop-jumping
involved to stay portable.

> 
> @@ -1680,7 +1680,7 @@ virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...)
>      va_list list;
>  
>      va_start(list, qemuCaps);
> -    virQEMUCapsSetVAList(qemuCaps, list);
> +    virQEMUCapsSetVAList(qemuCaps, &list);
>      va_end(list);

This usage is safe, because it is the address of a local variable. I
don't see any instance where you are taking the address of a parameter,
so while the patch is unusual, it doesn't look wrong.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList
Posted by Michal Privoznik 5 years ago
On 3/27/19 2:46 PM, Eric Blake wrote:
> On 3/27/19 7:02 AM, Andrea Bolognani wrote:
>> On Wed, 2019-03-27 at 10:50 +0100, Michal Privoznik wrote:
>>> There is one specific caller (testInfoSetArgs() in
>>> qemuxml2argvtest.c) which expect the  va_list argument to change
>>
>> s/  / /
>>
>>> after returning from the virQEMUCapsSetVAList() function.
>>> However, since we are passing plain va_list this is not
>>> guaranteed. The man page of stdarg(3) says:
>>>
>>>    If ap is passed to a function that uses va_arg(ap,type), then
>>>    the value of ap is undefined after the return of that function.
>>>
>>> (ap is a variable of type va_list)
> 
>> however I'd like to hear Eric's opinion before this gets merged.
> 
> Passing a va_list *arg as a parameter is doable, but it comes with odd
> effects. That is because the C standard permits both of the following
> implementation styles:
> 
> typedef struct __something *va_list;
> typedef struct __something va_list[];
> 
> but you get different semantics on what happens if you try to take the
> address of a va_list parameter, (C parameter lists undergo pointer
> decay, and taking the address of a parameter results in either the
> address of a pointer or the address of the first member of an array -
> which are different levels of dereferencing and thus different types).
> The type 'pointer to va_list' is sane, and the computation of
> '&local_va_list' is sane; it is only '&parameter_va_list' that gets you
> in trouble.  Here's where I demonstrated it further for the qemu list:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg00171.html
> 
> The only portable way to take va_list from one function to pass to a
> va_list * of another function is to va_copy() from the parameter into a
> local va_list, then take the address of the local.  But if you are the
> original owner of a local va_list (because your function took ...
> instead of va_list), you don't have to worry about the hoop-jumping
> involved to stay portable.
> 
>>
>> @@ -1680,7 +1680,7 @@ virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...)
>>       va_list list;
>>   
>>       va_start(list, qemuCaps);
>> -    virQEMUCapsSetVAList(qemuCaps, list);
>> +    virQEMUCapsSetVAList(qemuCaps, &list);
>>       va_end(list);
> 
> This usage is safe, because it is the address of a local variable. I
> don't see any instance where you are taking the address of a parameter,
> so while the patch is unusual, it doesn't look wrong.
> 

Okay, thanks for detailed explanation. I'll probably go with what Dan 
sugested and make virQEMUCapsSetVAList inline then. It looks safer and 
more portable.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList
Posted by Michal Privoznik 5 years ago
On 3/27/19 4:49 PM, Michal Privoznik wrote:

> 
> Okay, thanks for detailed explanation. I'll probably go with what Dan 
> sugested and make virQEMUCapsSetVAList inline then. It looks safer and 
> more portable.

Actually, that does not work. Because even if I mark the function 
'inline' gcc still produces assembly code that calls the function. So 
I've tried to come around that using attribute always_inline. But for 
some reason it is still not working.

I guess at this point, it's the best to just drop the 
virQEMUCapsSetVAList() completely.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList
Posted by Daniel P. Berrangé 5 years ago
On Wed, Mar 27, 2019 at 05:13:59PM +0100, Michal Privoznik wrote:
> On 3/27/19 4:49 PM, Michal Privoznik wrote:
> 
> > 
> > Okay, thanks for detailed explanation. I'll probably go with what Dan
> > sugested and make virQEMUCapsSetVAList inline then. It looks safer and
> > more portable.
> 
> Actually, that does not work. Because even if I mark the function 'inline'
> gcc still produces assembly code that calls the function. So I've tried to
> come around that using attribute always_inline. But for some reason it is
> still not working.
> 
> I guess at this point, it's the best to just drop the virQEMUCapsSetVAList()
> completely.

Yeah I should have been clearer. WHen I said "inline" virQEMUCapsSetVAList,
I really did mean drop this function entirely & copy its 2 lines of code
into the caller.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list