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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list