[PATCH 0/6] qemuBuildHostNetProps: Properly handle unsupported netdevs and remove virJSONValueObjectAppendStringPrintf
First patch fixes a crasher if unsupported network device is used and subsequently virJSONValueObjectAppendStringPrintf is removed. Peter Krempa (6): qemuBuildHostNetProps: Report proper errors for unhandled interface types qemuBuildHostNetProps: Don't use virJSONValueObjectAppendStringPrintf to format address qemuBuildChannelGuestfwdNetdevProps: Don't use virJSONValueObjectAppendStringPrintf qemuBuildHostNetProps: Append ipv6 address using virJSONValueObjectAdd qemuBuildHostNetProps: Append aliases without virJSONValueObjectAppendStringPrintf util: json: Remove unused virJSONValueObjectAppendStringPrintf src/libvirt_private.syms | 1 - src/qemu/qemu_command.c | 113 +++++++++++++++++++++++++-------------- src/util/virjson.c | 17 ------ src/util/virjson.h | 6 --- 4 files changed, 74 insertions(+), 63 deletions(-) -- 2.39.2
VIR_DOMAIN_NET_TYPE_NULL and VIR_DOMAIN_NET_TYPE_VDS are not implemented for the qemu driver but the formatter code in 'qemuBuildHostNetProps' didn't report an error for them and didn't even return from the function when they were encountered. This caused a crash in 'virJSONValueObjectAppendStringPrintf' which does not tolerate NULL JSON object to append to when the unsupported devices were used. Properly report error when unhandled devices are encountered. This also includes the case for VIR_DOMAIN_NET_TYPE_HOSTDEV, but that code path should never be reached. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2175582 Fixes: 6457619d186 Fixes: 0225483adce Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4839d45a34..589ec3e639 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3995,8 +3995,14 @@ qemuBuildHostNetProps(virDomainObj *vm, /* Should have been handled earlier via PCI/USB hotplug code. */ case VIR_DOMAIN_NET_TYPE_NULL: case VIR_DOMAIN_NET_TYPE_VDS: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("network device type '%s' is not supported by this hypervisor"), + virDomainNetTypeToString(netType)); + return NULL; + case VIR_DOMAIN_NET_TYPE_LAST: - break; + virReportEnumRangeError(virDomainNetType, netType); + return NULL; } if (virJSONValueObjectAppendStringPrintf(netprops, "id", "host%s", net->info.alias) < 0) -- 2.39.2
On Mon, Mar 06, 2023 at 10:20:30AM +0100, Peter Krempa wrote:
VIR_DOMAIN_NET_TYPE_NULL and VIR_DOMAIN_NET_TYPE_VDS are not implemented for the qemu driver but the formatter code in 'qemuBuildHostNetProps' didn't report an error for them and didn't even return from the function when they were encountered.
This caused a crash in 'virJSONValueObjectAppendStringPrintf' which does not tolerate NULL JSON object to append to when the unsupported devices were used.
Properly report error when unhandled devices are encountered. This also includes the case for VIR_DOMAIN_NET_TYPE_HOSTDEV, but that code path should never be reached.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2175582 Fixes: 6457619d186
Technically bac6b266fb6a was the first offender ;)
Fixes: 0225483adce Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4839d45a34..589ec3e639 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3995,8 +3995,14 @@ qemuBuildHostNetProps(virDomainObj *vm, /* Should have been handled earlier via PCI/USB hotplug code. */ case VIR_DOMAIN_NET_TYPE_NULL: case VIR_DOMAIN_NET_TYPE_VDS: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("network device type '%s' is not supported by this hypervisor"), + virDomainNetTypeToString(netType)); + return NULL; + case VIR_DOMAIN_NET_TYPE_LAST: - break; + virReportEnumRangeError(virDomainNetType, netType); + return NULL; }
if (virJSONValueObjectAppendStringPrintf(netprops, "id", "host%s", net->info.alias) < 0) -- 2.39.2
Prefer virJSONValueObjectAdd which we already use internally combined with local formatting of the string. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 68 +++++++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 589ec3e639..784003fbf5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3804,6 +3804,15 @@ qemuBuildNicDevProps(virDomainDef *def, } +static char * +qemuBuildHostNetSocketAddr(virDomainNetDef *net) +{ + return g_strdup_printf("%s:%d", + NULLSTR_EMPTY(net->data.socket.address), + net->data.socket.port); +} + + virJSONValue * qemuBuildHostNetProps(virDomainObj *vm, virDomainNetDef *net) @@ -3885,40 +3894,53 @@ qemuBuildHostNetProps(virDomainObj *vm, } break; - case VIR_DOMAIN_NET_TYPE_CLIENT: - if (virJSONValueObjectAdd(&netprops, "s:type", "socket", NULL) < 0 || - virJSONValueObjectAppendStringPrintf(netprops, "connect", "%s:%d", - net->data.socket.address, - net->data.socket.port) < 0) + case VIR_DOMAIN_NET_TYPE_CLIENT: { + g_autofree char *addr = qemuBuildHostNetSocketAddr(net); + + if (virJSONValueObjectAdd(&netprops, + "s:type", "socket", + "s:connect", addr, + NULL) < 0) return NULL; break; + } - case VIR_DOMAIN_NET_TYPE_SERVER: - if (virJSONValueObjectAdd(&netprops, "s:type", "socket", NULL) < 0 || - virJSONValueObjectAppendStringPrintf(netprops, "listen", "%s:%d", - NULLSTR_EMPTY(net->data.socket.address), - net->data.socket.port) < 0) + case VIR_DOMAIN_NET_TYPE_SERVER: { + g_autofree char *addr = qemuBuildHostNetSocketAddr(net); + + if (virJSONValueObjectAdd(&netprops, + "s:type", "socket", + "s:listen", addr, + NULL) < 0) return NULL; break; + } - case VIR_DOMAIN_NET_TYPE_MCAST: - if (virJSONValueObjectAdd(&netprops, "s:type", "socket", NULL) < 0 || - virJSONValueObjectAppendStringPrintf(netprops, "mcast", "%s:%d", - net->data.socket.address, - net->data.socket.port) < 0) + case VIR_DOMAIN_NET_TYPE_MCAST: { + g_autofree char *addr = qemuBuildHostNetSocketAddr(net); + + if (virJSONValueObjectAdd(&netprops, + "s:type", "socket", + "s:mcast", addr, + NULL) < 0) return NULL; break; + } - case VIR_DOMAIN_NET_TYPE_UDP: - if (virJSONValueObjectAdd(&netprops, "s:type", "socket", NULL) < 0 || - virJSONValueObjectAppendStringPrintf(netprops, "udp", "%s:%d", - net->data.socket.address, - net->data.socket.port) < 0 || - virJSONValueObjectAppendStringPrintf(netprops, "localaddr", "%s:%d", - net->data.socket.localaddr, - net->data.socket.localport) < 0) + case VIR_DOMAIN_NET_TYPE_UDP: { + g_autofree char *addr = qemuBuildHostNetSocketAddr(net); + g_autofree char *localaddr = g_strdup_printf("%s:%d", + net->data.socket.localaddr, + net->data.socket.localport); + + if (virJSONValueObjectAdd(&netprops, + "s:type", "socket", + "s:udp", addr, + "s:localaddr", localaddr, + NULL) < 0) return NULL; break; + } case VIR_DOMAIN_NET_TYPE_USER: if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) { -- 2.39.2
Use virJSONValueObjectAdd and format the string directly via g_strdup_printf. In the end virJSONValueObjectAppendStringPrintf will be removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 784003fbf5..420fe4ed0c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10475,22 +10475,24 @@ qemuBuildParallelChrDeviceProps(virDomainChrDef *chr) virJSONValue * qemuBuildChannelGuestfwdNetdevProps(virDomainChrDef *chr) { + g_autofree char *guestfwdstr = NULL; + g_autoptr(virJSONValue) guestfwdstrobj = NULL; g_autoptr(virJSONValue) guestfwdarr = virJSONValueNewArray(); - g_autoptr(virJSONValue) guestfwdstrobj = virJSONValueNewObject(); g_autofree char *addr = NULL; virJSONValue *ret = NULL; if (!(addr = virSocketAddrFormat(chr->target.addr))) return NULL; + guestfwdstr = g_strdup_printf("tcp:%s:%i-chardev:char%s", + addr, + virSocketAddrGetPort(chr->target.addr), + chr->info.alias); + /* this may seem weird, but qemu indeed decided that 'guestfwd' parameter * is an array of objects which have just one member named 'str' which * contains the description */ - if (virJSONValueObjectAppendStringPrintf(guestfwdstrobj, "str", - "tcp:%s:%i-chardev:char%s", - addr, - virSocketAddrGetPort(chr->target.addr), - chr->info.alias) < 0) + if (virJSONValueObjectAdd(&guestfwdstrobj, "s:str", guestfwdstr, NULL) < 0) return NULL; if (virJSONValueArrayAppend(guestfwdarr, &guestfwdstrobj) < 0) -- 2.39.2
The 'ipv6-prefix' and 'ipv6-prefixlen' fields can be directly added using virJSONValueObjectAdd rather tha by two separate calls. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 420fe4ed0c..1fc235153e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3974,11 +3974,10 @@ qemuBuildHostNetProps(virDomainObj *vm, if (virJSONValueObjectAppendString(netprops, "net", ipv4netaddr) < 0) return NULL; } else if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6)) { - if (virJSONValueObjectAppendString(netprops, "ipv6-prefix", addr) < 0) - return NULL; - if (ip->prefix && - virJSONValueObjectAppendNumberUlong(netprops, "ipv6-prefixlen", - ip->prefix) < 0) + if (virJSONValueObjectAdd(&netprops, + "s:ipv6-prefix", addr, + "p:ipv6-prefixlen", ip->prefix, + NULL) < 0) return NULL; } } -- 2.39.2
On a Monday in 2023, Peter Krempa wrote:
The 'ipv6-prefix' and 'ipv6-prefixlen' fields can be directly added using virJSONValueObjectAdd rather tha by two separate calls.
*than
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Format aliases into temporary strings and append them using virJSONValueObjectAdd. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1fc235153e..e5c093e7ea 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3821,6 +3821,7 @@ qemuBuildHostNetProps(virDomainObj *vm, size_t i; qemuDomainNetworkPrivate *netpriv = QEMU_DOMAIN_NETWORK_PRIVATE(net); g_autoptr(virJSONValue) netprops = NULL; + g_autofree char *alias = g_strdup_printf("host%s", net->info.alias); if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -3989,15 +3990,20 @@ qemuBuildHostNetProps(virDomainObj *vm, return NULL; break; - case VIR_DOMAIN_NET_TYPE_VHOSTUSER: - if (virJSONValueObjectAdd(&netprops, "s:type", "vhost-user", NULL) < 0 || - virJSONValueObjectAppendStringPrintf(netprops, "chardev", "char%s", net->info.alias) < 0) + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: { + g_autofree char *charalias = g_strdup_printf("char%s", net->info.alias); + + if (virJSONValueObjectAdd(&netprops, + "s:type", "vhost-user", + "s:chardev", charalias, + NULL) < 0) return NULL; if (net->driver.virtio.queues > 1 && virJSONValueObjectAppendNumberUlong(netprops, "queues", net->driver.virtio.queues) < 0) return NULL; break; + } case VIR_DOMAIN_NET_TYPE_VDPA: /* Caller will pass the fd to qemu with add-fd */ @@ -4026,7 +4032,7 @@ qemuBuildHostNetProps(virDomainObj *vm, return NULL; } - if (virJSONValueObjectAppendStringPrintf(netprops, "id", "host%s", net->info.alias) < 0) + if (virJSONValueObjectAdd(&netprops, "s:id", alias, NULL) < 0) return NULL; return g_steal_pointer(&netprops); -- 2.39.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virjson.c | 17 ----------------- src/util/virjson.h | 6 ------ 3 files changed, 24 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b249dcc85c..8792bf7f54 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2621,7 +2621,6 @@ virJSONValueObjectAppendNumberLong; virJSONValueObjectAppendNumberUint; virJSONValueObjectAppendNumberUlong; virJSONValueObjectAppendString; -virJSONValueObjectAppendStringPrintf; virJSONValueObjectDeflatten; virJSONValueObjectForeachKeyValue; virJSONValueObjectGet; diff --git a/src/util/virjson.c b/src/util/virjson.c index 1c3a50440e..4e822063bc 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -609,23 +609,6 @@ virJSONValueObjectAppendString(virJSONValue *object, } -int -virJSONValueObjectAppendStringPrintf(virJSONValue *object, - const char *key, - const char *fmt, - ...) -{ - va_list ap; - g_autofree char *str = NULL; - - va_start(ap, fmt); - str = g_strdup_vprintf(fmt, ap); - va_end(ap); - - return virJSONValueObjectInsertString(object, key, str, false); -} - - int virJSONValueObjectPrependString(virJSONValue *object, const char *key, diff --git a/src/util/virjson.h b/src/util/virjson.h index 0ea7caad23..95b8b14ae6 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -204,12 +204,6 @@ virJSONValueObjectAppendString(virJSONValue *object, const char *key, const char *value); int -virJSONValueObjectAppendStringPrintf(virJSONValue *object, - const char *key, - const char *fmt, - ...) - G_GNUC_PRINTF(3, 4); -int virJSONValueObjectPrependString(virJSONValue *object, const char *key, const char *value); -- 2.39.2
On a Monday in 2023, Peter Krempa wrote:
First patch fixes a crasher if unsupported network device is used and subsequently virJSONValueObjectAppendStringPrintf is removed.
Peter Krempa (6): qemuBuildHostNetProps: Report proper errors for unhandled interface types qemuBuildHostNetProps: Don't use virJSONValueObjectAppendStringPrintf to format address qemuBuildChannelGuestfwdNetdevProps: Don't use virJSONValueObjectAppendStringPrintf qemuBuildHostNetProps: Append ipv6 address using virJSONValueObjectAdd qemuBuildHostNetProps: Append aliases without virJSONValueObjectAppendStringPrintf util: json: Remove unused virJSONValueObjectAppendStringPrintf
src/libvirt_private.syms | 1 - src/qemu/qemu_command.c | 113 +++++++++++++++++++++++++-------------- src/util/virjson.c | 17 ------ src/util/virjson.h | 6 --- 4 files changed, 74 insertions(+), 63 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko -
Martin Kletzander -
Peter Krempa