From d6064e2759a24e0802f363e3a810dc5a7d7ebb15 Mon Sep 17 00:00:00 2001 From: Erik Skultety Date: Mon, 18 Nov 2019 12:04:16 +0100 Subject: [PATCH] libvirt-: Check caller-provided buffers to be NULL with size > 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. https://bugzilla.redhat.com/show_bug.cgi?id=1772842 Signed-off-by: Erik Skultety Reviewed-by: Daniel P. Berrangé --- 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 2d9c4061e2..51fb79cddd 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) {