4:50 a.m.
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(a)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
7:02 a.m.
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(a)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(a)redhat.com>
however I'd like to hear Eric's opinion before this gets merged.
--
Andrea Bolognani / Red Hat / Virtualization
7:16 a.m.
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(a)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(a)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 :|
8:46 a.m.
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
10:49 a.m.
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
11:13 a.m.
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
11:35 a.m.
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 :|
2103
days inactive
2103
days old
6 comments
4 participants
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Eric Blake -
Michal Privoznik