[libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList
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
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
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 :|
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 '¶meter_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
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 '¶meter_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
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
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 :|
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Eric Blake -
Michal Privoznik