[libvirt] [PATCH v2 0/4] Fix remote dispatch code trying to allocate 0-sized return buffers
This happens because of the switch to glib whose method g_malloc0 actually correctly returns NULL on size 0 which doesn't make sense to do. The outcome of that is that because virAllocN always returns 0, the dispatch code never fails allocation and passes the NULL pointer to the server-side public API which actually checks that and fails. https://bugzilla.redhat.com/show_bug.cgi?id=1772842 Since v1: - reworked the original patch 3/3 (now 4/4) by introducing a virCheckNonNull-style macro which checks whether the caller-provided array isn't by any chance NULL while the declared size of this array is a positive int (Dan's suggestion) -> apart from the single scenario check (NULL && n > 0), the other combinations are allowed for the legacy APIs letting the server-side handle this - added patch 3/4 when I realized one of the legacy APIs which didn't have the dispatch code autogenerated needed adjustment too - I deliberately didn't touch the APIs using typed params, as half of them are setters and the other half is supposed to be called twice, so we already have various NULL checks in place - there were a few legacy APIs that already had some protection in the remote dispatch code, so I didn't touch the client side for those, IMO we can always treat those on case-by-case basis if a similar issue arises than risk breaking one third of libvirt now :P Erik Skultety (4): rpc: gendispatch: Fix a couple of places adding trailing spaces rpc: gendispatch: Add a check for zero size client-side buffers remote: Add a check for zero sized client-side buffers libvirt-<module>: Check caller-provided buffers to be NULL with size > 0 src/internal.h | 13 +++++++++++++ src/libvirt-domain-snapshot.c | 4 ++-- src/libvirt-domain.c | 21 ++++++--------------- src/libvirt-host.c | 2 +- src/libvirt-interface.c | 4 ++-- src/libvirt-network.c | 4 ++-- src/libvirt-nodedev.c | 4 ++-- src/libvirt-nwfilter.c | 2 +- src/libvirt-secret.c | 2 +- src/libvirt-storage.c | 6 +++--- src/remote/remote_daemon_dispatch.c | 6 ++++++ src/rpc/gendispatch.pl | 12 ++++++++++-- 12 files changed, 49 insertions(+), 31 deletions(-) -- 2.23.0
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/rpc/gendispatch.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 7c868191d1..8656c8f205 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -1062,7 +1062,7 @@ elsif ($mode eq "server") { print "\n"; print " $conn_type $conn_var = $conn_method(client);\n"; - print " if (!$conn_var) \n"; + print " if (!$conn_var)\n"; print " goto cleanup;\n"; print "\n"; @@ -1144,7 +1144,7 @@ elsif ($mode eq "server") { print "\n"; } else { if ($modern_ret_as_list) { - print " if ((nresults = \n"; + print " if ((nresults =\n"; my $indln = " $prefix$call->{ProcName}("; print $indln; print join(",\n" . ' ' x (length $indln), @args_list); -- 2.23.0
On Thu, Nov 21, 2019 at 09:58:29AM +0100, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/rpc/gendispatch.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
After libvirt switched to GLib, we also started to use glib allocation primitives as of commit e85e34f3. Unlike malloc which is ambiguous with regards to size == 0 (which in our case returned a unique pointer safe to be passed to free), g_malloc0 strictly returns NULL on size == 0. This change broke our legacy APIs which rely on the caller to pre-allocate the target buffer to hold the results and pass the buffer size as an argument. Since it makes very little sense to call an API with nowhere to store the results, fix this by returning 0 directly in such case in the RPC dispatch code - there are modern API equivalents allocating the target buffer automatically anyway. https://bugzilla.redhat.com/show_bug.cgi?id=1772842 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/rpc/gendispatch.pl | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 8656c8f205..524d31f741 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -1073,6 +1073,14 @@ elsif ($mode eq "server") { print " goto cleanup;\n"; print " }\n"; print "\n"; + + + print " if (args->$single_ret_list_max_var == 0) {\n"; + print " ret->$single_ret_list_name.${single_ret_list_name}_len = 0;\n"; + print " rv = 0;\n"; + print " goto cleanup;\n"; + print " }\n"; + print "\n"; } print join("\n", @getters_list); -- 2.23.0
On Thu, Nov 21, 2019 at 09:58:30AM +0100, Erik Skultety wrote:
After libvirt switched to GLib, we also started to use glib allocation primitives as of commit e85e34f3. Unlike malloc which is ambiguous with regards to size == 0 (which in our case returned a unique pointer safe to be passed to free), g_malloc0 strictly returns NULL on size == 0.
This change broke our legacy APIs which rely on the caller to pre-allocate the target buffer to hold the results and pass the buffer size as an argument. Since it makes very little sense to call an API with nowhere to store the results, fix this by returning 0 directly in such case in the RPC dispatch code - there are modern API equivalents allocating the target buffer automatically anyway.
As mentioned before, I don't think we should be short-circuiting the driver APIs like this. It is still valid to call the APIs with zero length list such that we get access control checks validated.
https://bugzilla.redhat.com/show_bug.cgi?id=1772842
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/rpc/gendispatch.pl | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 8656c8f205..524d31f741 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -1073,6 +1073,14 @@ elsif ($mode eq "server") { print " goto cleanup;\n"; print " }\n"; print "\n"; + + + print " if (args->$single_ret_list_max_var == 0) {\n"; + print " ret->$single_ret_list_name.${single_ret_list_name}_len = 0;\n"; + print " rv = 0;\n"; + print " goto cleanup;\n"; + print " }\n"; + print "\n"; }
print join("\n", @getters_list); -- 2.23.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
Adjustment to the dispatch code which is not generated by gendispatch.pl. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/remote/remote_daemon_dispatch.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index f369ffb02a..1d75ec3d37 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -2753,6 +2753,12 @@ remoteDispatchDomainGetVcpuPinInfo(virNetServerPtr server G_GNUC_UNUSED, goto cleanup; } + if (args->ncpumaps == 0) { + ret->cpumaps.cpumaps_len = 0; + rv = 0; + goto cleanup; + } + if (INT_MULTIPLY_OVERFLOW(args->ncpumaps, args->maplen) || args->ncpumaps * args->maplen > REMOTE_CPUMAPS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("maxinfo * maplen > REMOTE_CPUMAPS_MAX")); -- 2.23.0
On Thu, Nov 21, 2019 at 09:58:31AM +0100, Erik Skultety wrote:
Adjustment to the dispatch code which is not generated by gendispatch.pl.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/remote/remote_daemon_dispatch.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index f369ffb02a..1d75ec3d37 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -2753,6 +2753,12 @@ remoteDispatchDomainGetVcpuPinInfo(virNetServerPtr server G_GNUC_UNUSED, goto cleanup; }
+ if (args->ncpumaps == 0) { + ret->cpumaps.cpumaps_len = 0; + rv = 0; + goto cleanup; + }
Same comment as previous patch, I don't think we should be short-circuiting the drivers in our dispatch code. Patch 4 makes this redundant by fixing the root cause bug. 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 :|
Pre-Glib era which used malloc allowed the size of the client-side buffers to be declared as 0, because malloc documents that it can either return 0 or a unique pointer on 0 size allocations. With glib this doesn't work anymore, because glib documents that for such allocation requests NULL is always returned which results in an error in our public API checks server-side. This patch complements the fix in the RPC layer by explicitly erroring out on the following combination of args used by our legacy APIs (their moder equivalents don't suffer from this): function(caller-allocated-array, size, ...) { if (!caller-allocated-array && size > 0) return error; } treating everything else as a valid input and potentially let that fail on the server-side rather than client-side. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/internal.h | 13 +++++++++++++ src/libvirt-domain-snapshot.c | 4 ++-- src/libvirt-domain.c | 21 ++++++--------------- src/libvirt-host.c | 2 +- src/libvirt-interface.c | 4 ++-- src/libvirt-network.c | 4 ++-- src/libvirt-nodedev.c | 4 ++-- src/libvirt-nwfilter.c | 2 +- src/libvirt-secret.c | 2 +- src/libvirt-storage.c | 6 +++--- 10 files changed, 33 insertions(+), 29 deletions(-) diff --git a/src/internal.h b/src/internal.h index 0ff9f496ac..bcc5a1c157 100644 --- a/src/internal.h +++ b/src/internal.h @@ -429,6 +429,19 @@ } \ } while (0) +/* This check is intended to be used with legacy APIs only which expect the + * caller to pre-allocate the target buffer. + * We want to allow callers pass NULL arrays if the size is declared as 0 and + * still succeed in calling the API. + */ +#define virCheckNonNullArrayArgGoto(argname, argsize, label) \ + do { \ + if (!argname && argsize > 0) { \ + virReportInvalidNonNullArg(argname); \ + goto label; \ + } \ + } while (0) + /* Count leading zeros in an unsigned int. * diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 20a3bc5545..33593e11e9 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -398,7 +398,7 @@ virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, virCheckDomainReturn(domain, -1); conn = domain->conn; - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, nameslen, error); virCheckNonNegativeArgGoto(nameslen, error); if (conn->driver->domainSnapshotListNames) { @@ -600,7 +600,7 @@ virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, virCheckDomainSnapshotReturn(snapshot, -1); conn = snapshot->domain->conn; - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, nameslen, error); virCheckNonNegativeArgGoto(nameslen, error); if (conn->driver->domainSnapshotListChildrenNames) { diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 02622cb2ca..17cdd5bcaf 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -59,7 +59,7 @@ virConnectListDomains(virConnectPtr conn, int *ids, int maxids) virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(ids, error); + virCheckNonNullArrayArgGoto(ids, maxids, error); virCheckNonNegativeArgGoto(maxids, error); if (conn->driver->connectListDomains) { @@ -6386,7 +6386,7 @@ virConnectListDefinedDomains(virConnectPtr conn, char **const names, virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (conn->driver->connectListDefinedDomains) { @@ -7298,7 +7298,7 @@ virDomainGetVcpuPinInfo(virDomainPtr domain, int ncpumaps, virCheckDomainReturn(domain, -1); conn = domain->conn; - virCheckNonNullArgGoto(cpumaps, error); + virCheckNonNullArrayArgGoto(cpumaps, ncpumaps, error); virCheckPositiveArgGoto(ncpumaps, error); virCheckPositiveArgGoto(maplen, error); @@ -10996,10 +10996,7 @@ virDomainGetDiskErrors(virDomainPtr dom, virCheckDomainReturn(dom, -1); - if (maxerrors) - virCheckNonNullArgGoto(errors, error); - else - virCheckNullArgGoto(errors, error); + virCheckNonNullArrayArgGoto(errors, maxerrors, error); if (dom->conn->driver->domainGetDiskErrors) { int ret = dom->conn->driver->domainGetDiskErrors(dom, errors, @@ -11136,10 +11133,7 @@ virDomainFSFreeze(virDomainPtr dom, virCheckDomainReturn(dom, -1); virCheckReadOnlyGoto(dom->conn->flags, error); - if (nmountpoints) - virCheckNonNullArgGoto(mountpoints, error); - else - virCheckNullArgGoto(mountpoints, error); + virCheckNonNullArrayArgGoto(mountpoints, nmountpoints, error); if (dom->conn->driver->domainFSFreeze) { int ret = dom->conn->driver->domainFSFreeze( @@ -11181,10 +11175,7 @@ virDomainFSThaw(virDomainPtr dom, virCheckDomainReturn(dom, -1); virCheckReadOnlyGoto(dom->conn->flags, error); - if (nmountpoints) - virCheckNonNullArgGoto(mountpoints, error); - else - virCheckNullArgGoto(mountpoints, error); + virCheckNonNullArrayArgGoto(mountpoints, nmountpoints, error); if (dom->conn->driver->domainFSThaw) { int ret = dom->conn->driver->domainFSThaw( diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 221a1b7a43..94ba5a8e80 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -910,7 +910,7 @@ virNodeGetCellsFreeMemory(virConnectPtr conn, unsigned long long *freeMems, virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(freeMems, error); + virCheckNonNullArrayArgGoto(freeMems, maxCells, error); virCheckPositiveArgGoto(maxCells, error); virCheckNonNegativeArgGoto(startCell, error); diff --git a/src/libvirt-interface.c b/src/libvirt-interface.c index 7228ddca57..2d2df68131 100644 --- a/src/libvirt-interface.c +++ b/src/libvirt-interface.c @@ -166,7 +166,7 @@ virConnectListInterfaces(virConnectPtr conn, char **const names, int maxnames) virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (conn->interfaceDriver && conn->interfaceDriver->connectListInterfaces) { @@ -245,7 +245,7 @@ virConnectListDefinedInterfaces(virConnectPtr conn, virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (conn->interfaceDriver && conn->interfaceDriver->connectListDefinedInterfaces) { diff --git a/src/libvirt-network.c b/src/libvirt-network.c index 146ccc5e4a..09e24fb0a8 100644 --- a/src/libvirt-network.c +++ b/src/libvirt-network.c @@ -175,7 +175,7 @@ virConnectListNetworks(virConnectPtr conn, char **const names, int maxnames) virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (conn->networkDriver && conn->networkDriver->connectListNetworks) { @@ -252,7 +252,7 @@ virConnectListDefinedNetworks(virConnectPtr conn, char **const names, virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (conn->networkDriver && conn->networkDriver->connectListDefinedNetworks) { diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 10050b193b..dce46b7181 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -169,7 +169,7 @@ virNodeListDevices(virConnectPtr conn, virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (conn->nodeDeviceDriver && conn->nodeDeviceDriver->nodeListDevices) { @@ -415,7 +415,7 @@ virNodeDeviceListCaps(virNodeDevicePtr dev, virResetLastError(); virCheckNodeDeviceReturn(dev, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (dev->conn->nodeDeviceDriver && dev->conn->nodeDeviceDriver->nodeDeviceListCaps) { diff --git a/src/libvirt-nwfilter.c b/src/libvirt-nwfilter.c index 16eceb6525..d28220db8a 100644 --- a/src/libvirt-nwfilter.c +++ b/src/libvirt-nwfilter.c @@ -127,7 +127,7 @@ virConnectListNWFilters(virConnectPtr conn, char **const names, int maxnames) virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (conn->nwfilterDriver && conn->nwfilterDriver->connectListNWFilters) { diff --git a/src/libvirt-secret.c b/src/libvirt-secret.c index 711c4fc580..33cbdd7b0b 100644 --- a/src/libvirt-secret.c +++ b/src/libvirt-secret.c @@ -166,7 +166,7 @@ virConnectListSecrets(virConnectPtr conn, char **uuids, int maxuuids) virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(uuids, error); + virCheckNonNullArrayArgGoto(uuids, maxuuids, error); virCheckNonNegativeArgGoto(maxuuids, error); if (conn->secretDriver != NULL && conn->secretDriver->connectListSecrets != NULL) { diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c index 05b2365692..0406fe84d3 100644 --- a/src/libvirt-storage.c +++ b/src/libvirt-storage.c @@ -197,7 +197,7 @@ virConnectListStoragePools(virConnectPtr conn, virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (conn->storageDriver && conn->storageDriver->connectListStoragePools) { @@ -277,7 +277,7 @@ virConnectListDefinedStoragePools(virConnectPtr conn, virResetLastError(); virCheckConnectReturn(conn, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (conn->storageDriver && conn->storageDriver->connectListDefinedStoragePools) { @@ -1268,7 +1268,7 @@ virStoragePoolListVolumes(virStoragePoolPtr pool, virResetLastError(); virCheckStoragePoolReturn(pool, -1); - virCheckNonNullArgGoto(names, error); + virCheckNonNullArrayArgGoto(names, maxnames, error); virCheckNonNegativeArgGoto(maxnames, error); if (pool->conn->storageDriver && pool->conn->storageDriver->storagePoolListVolumes) { -- 2.23.0
On Thu, Nov 21, 2019 at 09:58:32AM +0100, Erik Skultety wrote:
Pre-Glib era which used malloc allowed the size of the client-side buffers to be declared as 0, because malloc documents that it can either return 0 or a unique pointer on 0 size allocations. With glib this doesn't work anymore, because glib documents that for such allocation requests NULL is always returned which results in an error in our public API checks server-side. This patch complements the fix in the RPC layer by explicitly erroring out on the following combination of args used by our legacy APIs (their moder equivalents don't suffer from this):
function(caller-allocated-array, size, ...) { if (!caller-allocated-array && size > 0) return error; }
treating everything else as a valid input and potentially let that fail on the server-side rather than client-side.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/internal.h | 13 +++++++++++++ src/libvirt-domain-snapshot.c | 4 ++-- src/libvirt-domain.c | 21 ++++++--------------- src/libvirt-host.c | 2 +- src/libvirt-interface.c | 4 ++-- src/libvirt-network.c | 4 ++-- src/libvirt-nodedev.c | 4 ++-- src/libvirt-nwfilter.c | 2 +- src/libvirt-secret.c | 2 +- src/libvirt-storage.c | 6 +++--- 10 files changed, 33 insertions(+), 29 deletions(-)
@@ -11136,10 +11133,7 @@ virDomainFSFreeze(virDomainPtr dom,
virCheckDomainReturn(dom, -1); virCheckReadOnlyGoto(dom->conn->flags, error); - if (nmountpoints) - virCheckNonNullArgGoto(mountpoints, error); - else - virCheckNullArgGoto(mountpoints, error);
Interesting, so this actually returned an error if you did *not* pass NULL when nmountpoints == 0, so this would be broken if the caller's malloc() returned non-NULL region for size==0
+ virCheckNonNullArrayArgGoto(mountpoints, nmountpoints, error);
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 (2)
-
Daniel P. Berrangé -
Erik Skultety