2:39 a.m.
I've pushed both patches, because they are trivial.
Michal Prívozník (2):
qemu_process.c: Fix VIR_QEMU_PROCESS_START_RESET_NVRAM value
qemu_process: Fix theoretical overflow in uint to bool typecast
src/qemu/qemu_process.c | 2 +-
src/qemu/qemu_process.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--
2.34.1
2:39 a.m.
New subject: [PATCH 1/2] qemu_process.c: Fix VIR_QEMU_PROCESS_START_RESET_NVRAM value
In one of recent commits qemuProcessStartFlags enum gained new
value: VIR_QEMU_PROCESS_START_RESET_NVRAM but due to a typo it
has the same value as another member of the enum. Fix that.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index f6c0d63d11..f6dd3f5104 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -79,7 +79,7 @@ typedef enum {
VIR_QEMU_PROCESS_START_PRETEND = 1 << 3,
VIR_QEMU_PROCESS_START_NEW = 1 << 4, /* internal, new VM is starting
*/
VIR_QEMU_PROCESS_START_GEN_VMID = 1 << 5, /* Generate a new VMID */
- VIR_QEMU_PROCESS_START_RESET_NVRAM = 1 << 5, /* Re-initialize NVRAM from
template */
+ VIR_QEMU_PROCESS_START_RESET_NVRAM = 1 << 6, /* Re-initialize NVRAM from
template */
} qemuProcessStartFlags;
int qemuProcessStart(virConnectPtr conn,
--
2.34.1
3:14 a.m.
New subject: [PATCH 1/2] qemu_process.c: Fix VIR_QEMU_PROCESS_START_RESET_NVRAM value
On Wed, Feb 09, 2022 at 09:39:49 +0100, Michal Privoznik wrote:
In one of recent commits qemuProcessStartFlags enum gained new
value: VIR_QEMU_PROCESS_START_RESET_NVRAM but due to a typo it
has the same value as another member of the enum. Fix that.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
3:41 a.m.
New subject: [PATCH 1/2] qemu_process.c: Fix VIR_QEMU_PROCESS_START_RESET_NVRAM value
On Wed, Feb 09, 2022 at 09:39:49AM +0100, Michal Privoznik wrote:
In one of recent commits qemuProcessStartFlags enum gained new
value: VIR_QEMU_PROCESS_START_RESET_NVRAM but due to a typo it
has the same value as another member of the enum. Fix that.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index f6c0d63d11..f6dd3f5104 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -79,7 +79,7 @@ typedef enum {
VIR_QEMU_PROCESS_START_PRETEND = 1 << 3,
VIR_QEMU_PROCESS_START_NEW = 1 << 4, /* internal, new VM is starting
*/
VIR_QEMU_PROCESS_START_GEN_VMID = 1 << 5, /* Generate a new VMID */
- VIR_QEMU_PROCESS_START_RESET_NVRAM = 1 << 5, /* Re-initialize NVRAM from
template */
+ VIR_QEMU_PROCESS_START_RESET_NVRAM = 1 << 6, /* Re-initialize NVRAM from
template */
} qemuProcessStartFlags;
facepalm. Shame there's no compiler warning for this scenario. Though
we do it intentionally in a handful of places, it would be better to
warn by default and selectively hide it with a pragma.
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 :|
2:39 a.m.
New subject: [PATCH 2/2] qemu_process: Fix theoretical overflow in uint to bool typecast
The qemuPrepareNVRAM() function accepts three arguments and the
last one being a boolean type. However, when the function is
called from qemuProcessPrepareHost() the argument passed is a
result of logical and of @flags (unsigned int) and
VIR_QEMU_PROCESS_START_RESET_NVRAM value. In theory this is
unsafe to do because if the value of the flag is ever changed
then this expression might overflow. Do what we do elsewhere:
double negation.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7066696f31..24873f6fb7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6983,7 +6983,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver,
qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0)
return -1;
- if (qemuPrepareNVRAM(driver, vm, flags & VIR_QEMU_PROCESS_START_RESET_NVRAM) <
0)
+ if (qemuPrepareNVRAM(driver, vm, !!(flags & VIR_QEMU_PROCESS_START_RESET_NVRAM))
< 0)
return -1;
if (vm->def->vsock) {
--
2.34.1
3:08 a.m.
New subject: [PATCH 2/2] qemu_process: Fix theoretical overflow in uint to bool typecast
On Wed, Feb 09, 2022 at 09:39:50 +0100, Michal Privoznik wrote:
The qemuPrepareNVRAM() function accepts three arguments and the
last one being a boolean type. However, when the function is
called from qemuProcessPrepareHost() the argument passed is a
result of logical and of @flags (unsigned int) and
VIR_QEMU_PROCESS_START_RESET_NVRAM value. In theory this is
unsafe to do because if the value of the flag is ever changed
I don't think that this is accurate as casting a unsigned int (32bits)
to a bool (8 bits) works properly even when the lower 8 bits of the
value equal to 0:
$ cat bool.c
#include <stdint.h>
#include <stdio.h>
#include <limits.h>
#include <stdbool.h>
void p(char *s, unsigned long long v, uint8_t u, bool b) {
printf("%30s: %30llu: %4u: %d\n", s, v, u, b);
}
#define E(val) \
p(#val, (val), (val), (val))
#define T(type) \
E(type - 1);\
E(type);\
E(type + 1)
int main() {
printf("bool size: %u\n", sizeof(bool));
E(0);
T(CHAR_MAX);
T(UCHAR_MAX);
T(UINT8_MAX);
T(INT_MAX);
T(UINT_MAX);
}
$ ./a.out
bool size: 1
0: 0: 0: 0
0x7f - 1: 126: 126: 1
0x7f: 127: 127: 1
0x7f + 1: 128: 128: 1
(0x7f * 2 + 1) - 1: 254: 254: 1
(0x7f * 2 + 1): 255: 255: 1
(0x7f * 2 + 1) + 1: 256: 0: 1
(255) - 1: 254: 254: 1
(255): 255: 255: 1
(255) + 1: 256: 0: 1
0x7fffffff - 1: 2147483646: 254: 1
0x7fffffff: 2147483647: 255: 1
0x7fffffff + 1: 18446744071562067968: 0: 1
(0x7fffffff * 2U + 1U) - 1: 4294967294: 254: 1
(0x7fffffff * 2U + 1U): 4294967295: 255: 1
(0x7fffffff * 2U + 1U) + 1: 0: 0: 0
then this expression might overflow. Do what we do elsewhere:
double negation.
I have no problem with the code change to make it use our common
pattern.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7066696f31..24873f6fb7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6983,7 +6983,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver,
qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0)
return -1;
- if (qemuPrepareNVRAM(driver, vm, flags & VIR_QEMU_PROCESS_START_RESET_NVRAM)
< 0)
+ if (qemuPrepareNVRAM(driver, vm, !!(flags & VIR_QEMU_PROCESS_START_RESET_NVRAM))
< 0)
return -1;
if (vm->def->vsock) {
--
2.34.1
3:26 a.m.
New subject: [PATCH 2/2] qemu_process: Fix theoretical overflow in uint to bool typecast
On 2/9/22 10:08, Peter Krempa wrote:
On Wed, Feb 09, 2022 at 09:39:50 +0100, Michal Privoznik wrote:
> The qemuPrepareNVRAM() function accepts three arguments and the
> last one being a boolean type. However, when the function is
> called from qemuProcessPrepareHost() the argument passed is a
> result of logical and of @flags (unsigned int) and
> VIR_QEMU_PROCESS_START_RESET_NVRAM value. In theory this is
> unsafe to do because if the value of the flag is ever changed
I don't think that this is accurate as casting a unsigned int (32bits)
to a bool (8 bits) works properly even when the lower 8 bits of the
value equal to 0:
$ cat bool.c
#include <stdint.h>
#include <stdio.h>
#include <limits.h>
#include <stdbool.h>
void p(char *s, unsigned long long v, uint8_t u, bool b) {
printf("%30s: %30llu: %4u: %d\n", s, v, u, b);
}
#define E(val) \
p(#val, (val), (val), (val))
#define T(type) \
E(type - 1);\
E(type);\
E(type + 1)
int main() {
printf("bool size: %u\n", sizeof(bool));
E(0);
T(CHAR_MAX);
T(UCHAR_MAX);
T(UINT8_MAX);
T(INT_MAX);
T(UINT_MAX);
}
$ ./a.out
bool size: 1
0: 0: 0: 0
0x7f - 1: 126: 126: 1
0x7f: 127: 127: 1
0x7f + 1: 128: 128: 1
(0x7f * 2 + 1) - 1: 254: 254: 1
(0x7f * 2 + 1): 255: 255: 1
(0x7f * 2 + 1) + 1: 256: 0: 1
(255) - 1: 254: 254: 1
(255): 255: 255: 1
(255) + 1: 256: 0: 1
0x7fffffff - 1: 2147483646: 254: 1
0x7fffffff: 2147483647: 255: 1
0x7fffffff + 1: 18446744071562067968: 0: 1
(0x7fffffff * 2U + 1U) - 1: 4294967294: 254: 1
(0x7fffffff * 2U + 1U): 4294967295: 255: 1
(0x7fffffff * 2U + 1U) + 1: 0: 0: 0
Ah, looks like GCC is clever enough. For the following code:
void pb(bool b) {
printf("%u\n", b);
}
int main(int argc, char *argv[])
{
unsigned int x = -1;
pb(x & (1 << (sizeof(bool) * 8)));
return 0;
}
GCC generates some extra code when calling the pb() function:
1172: c7 45 fc ff ff ff ff movl $0xffffffff,-0x4(%rbp)
1179: 8b 45 fc mov -0x4(%rbp),%eax
117c: 25 00 01 00 00 and $0x100,%eax
1181: 85 c0 test %eax,%eax
1183: 0f 95 c0 setne %al
1186: 0f b6 c0 movzbl %al,%eax
1189: 89 c7 mov %eax,%edi
118b: e8 a9 ff ff ff call 1139 <pb>
IOW, the compiler does "sign extension", well "reduction". Clang
produces similar assembler, except it makes doubly sure that bool has
just the lowest bit set:
401166: c7 45 ec ff ff ff ff movl $0xffffffff,-0x14(%rbp)
40116d: 8b 45 ec mov -0x14(%rbp),%eax
401170: 25 00 01 00 00 and $0x100,%eax
401175: 83 f8 00 cmp $0x0,%eax
401178: 0f 95 c0 setne %al
40117b: 0f b6 f8 movzbl %al,%edi
40117e: 83 e7 01 and $0x1,%edi
401181: e8 9a ff ff ff call 401120 <pb>
So, since compilers are now clever enough, I guess we can stop writing
the expression like I did.
Michal
1009
days inactive
1009
days old
6 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa