[libvirt PATCH 0/2] fixes allocation issues leading to crashes
Our libvirt-dbus tests discovered allocation issues introduced by recent rewrite to use g_new0 instead of VIR_ALLOC_N by allocating array one element shorter in some places which can lead to crashes of client applications counting on a fact that the returned arrays are NULL terminated. Pavel Hrdina (2): conf: fix g_new0 allocation conf: virsecretobj: fix g_new0 allocation src/conf/virinterfaceobj.c | 2 +- src/conf/virnetworkobj.c | 4 ++-- src/conf/virnodedeviceobj.c | 2 +- src/conf/virsecretobj.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) -- 2.26.2
Fixes commit <a5d88ffe0ad9b5d5314ab0058c5b363f9f79b8ee> which changed allocation from VIR_ALLOC_N to g_new0 but missed some +1 on number of allocated elements. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/virinterfaceobj.c | 2 +- src/conf/virnetworkobj.c | 4 ++-- src/conf/virnodedeviceobj.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 5a5fd65e14..e08b4b9b0f 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -320,7 +320,7 @@ virInterfaceObjListExport(virConnectPtr conn, virObjectRWLockRead(ifaceobjs); if (ifaces) - data.ifaces = g_new0(virInterfacePtr, virHashSize(ifaceobjs->objsName)); + data.ifaces = g_new0(virInterfacePtr, virHashSize(ifaceobjs->objsName) + 1); virHashForEach(ifaceobjs->objsName, virInterfaceObjListExportCallback, &data); diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 1fcfa9cdc2..a2835ebf8e 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1407,7 +1407,7 @@ virNetworkObjListExport(virConnectPtr conn, virObjectRWLockRead(netobjs); if (nets) - data.nets = g_new0(virNetworkPtr, virHashSize(netobjs->objs)); + data.nets = g_new0(virNetworkPtr, virHashSize(netobjs->objs) + 1); virHashForEach(netobjs->objs, virNetworkObjListExportCallback, &data); @@ -1801,7 +1801,7 @@ virNetworkObjPortListExport(virNetworkPtr net, if (ports) { *ports = NULL; - data.ports = g_new0(virNetworkPortPtr, virHashSize(obj->ports)); + data.ports = g_new0(virNetworkPortPtr, virHashSize(obj->ports) + 1); } virHashForEach(obj->ports, virNetworkObjPortListExportCallback, &data); diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 28905c67b6..9af80b8036 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -931,7 +931,7 @@ virNodeDeviceObjListExport(virConnectPtr conn, virObjectRWLockRead(devs); if (devices) - data.devices = g_new0(virNodeDevicePtr, virHashSize(devs->objs)); + data.devices = g_new0(virNodeDevicePtr, virHashSize(devs->objs) + 1); virHashForEach(devs->objs, virNodeDeviceObjListExportCallback, &data); virObjectRWUnlock(devs); -- 2.26.2
On 10/12/20 2:13 PM, Pavel Hrdina wrote:
Fixes commit <a5d88ffe0ad9b5d5314ab0058c5b363f9f79b8ee> which changed allocation from VIR_ALLOC_N to g_new0 but missed some +1 on number of allocated elements.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/virinterfaceobj.c | 2 +- src/conf/virnetworkobj.c | 4 ++-- src/conf/virnodedeviceobj.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 5a5fd65e14..e08b4b9b0f 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -320,7 +320,7 @@ virInterfaceObjListExport(virConnectPtr conn,
virObjectRWLockRead(ifaceobjs); if (ifaces) - data.ifaces = g_new0(virInterfacePtr, virHashSize(ifaceobjs->objsName)); + data.ifaces = g_new0(virInterfacePtr, virHashSize(ifaceobjs->objsName) + 1);
virHashForEach(ifaceobjs->objsName, virInterfaceObjListExportCallback, &data);
diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 1fcfa9cdc2..a2835ebf8e 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1407,7 +1407,7 @@ virNetworkObjListExport(virConnectPtr conn,
virObjectRWLockRead(netobjs); if (nets) - data.nets = g_new0(virNetworkPtr, virHashSize(netobjs->objs)); + data.nets = g_new0(virNetworkPtr, virHashSize(netobjs->objs) + 1);
virHashForEach(netobjs->objs, virNetworkObjListExportCallback, &data);
@@ -1801,7 +1801,7 @@ virNetworkObjPortListExport(virNetworkPtr net, if (ports) { *ports = NULL;
- data.ports = g_new0(virNetworkPortPtr, virHashSize(obj->ports)); + data.ports = g_new0(virNetworkPortPtr, virHashSize(obj->ports) + 1); }
virHashForEach(obj->ports, virNetworkObjPortListExportCallback, &data); diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 28905c67b6..9af80b8036 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -931,7 +931,7 @@ virNodeDeviceObjListExport(virConnectPtr conn,
virObjectRWLockRead(devs); if (devices) - data.devices = g_new0(virNodeDevicePtr, virHashSize(devs->objs)); + data.devices = g_new0(virNodeDevicePtr, virHashSize(devs->objs) + 1);
virHashForEach(devs->objs, virNodeDeviceObjListExportCallback, &data); virObjectRWUnlock(devs);
Reviewed-by: Laine Stump <laine@redhat.com>
Fixes commit <d5b05614dfbc9bd60ea1a31a9cc32aaf3c771ddc> which changed allocation from VIR_ALLOC_N to g_new0 but missed one +1 on number of allocated elements. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/virsecretobj.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index c98d52f1e4..d74deb9316 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -588,7 +588,7 @@ virSecretObjListExport(virConnectPtr conn, virObjectRWLockRead(secretobjs); if (secrets) - data.secrets = g_new0(virSecretPtr, virHashSize(secretobjs->objs)); + data.secrets = g_new0(virSecretPtr, virHashSize(secretobjs->objs) + 1); virHashForEach(secretobjs->objs, virSecretObjListExportCallback, &data); virObjectRWUnlock(secretobjs); -- 2.26.2
On 10/12/20 2:13 PM, Pavel Hrdina wrote:
Fixes commit <d5b05614dfbc9bd60ea1a31a9cc32aaf3c771ddc> which changed allocation from VIR_ALLOC_N to g_new0 but missed one +1 on number of allocated elements.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/virsecretobj.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index c98d52f1e4..d74deb9316 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -588,7 +588,7 @@ virSecretObjListExport(virConnectPtr conn,
virObjectRWLockRead(secretobjs); if (secrets) - data.secrets = g_new0(virSecretPtr, virHashSize(secretobjs->objs)); + data.secrets = g_new0(virSecretPtr, virHashSize(secretobjs->objs) + 1);
virHashForEach(secretobjs->objs, virSecretObjListExportCallback, &data); virObjectRWUnlock(secretobjs);
Reviewed-by: Laine Stump <laine@redhat.com>
On a Monday in 2020, Pavel Hrdina wrote:
Our libvirt-dbus tests discovered allocation issues introduced by recent rewrite to use g_new0 instead of VIR_ALLOC_N by allocating array one element shorter in some places which can lead to crashes of client applications counting on a fact that the returned arrays are NULL terminated.
Oops, I did not notice that my vim macro [0] dropped these (due to looking for the ')' from the left side). There seem to be no other cases like this in my changes where the expression contains a parenthesis. Thanks for fixing this. Jano
Pavel Hrdina (2): conf: fix g_new0 allocation conf: virsecretobj: fix g_new0 allocation
src/conf/virinterfaceobj.c | 2 +- src/conf/virnetworkobj.c | 4 ++-- src/conf/virnodedeviceobj.c | 2 +- src/conf/virsecretobj.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
[0] I was not able to replace everything with Coccinelle
participants (4)
-
Ján Tomko -
Laine Stump -
Laine Stump -
Pavel Hrdina