[libvirt] [PATCH 0/4] Couple of allocation fixes
These stem from me playing with OOM testing. I've discovered more bugs, but they're mostly in tests themselves, e.g. use without check. Michal Prívozník (4): virstorageobj: Don't clear vols if they weren't initialized virNetServerPreExecRestart: Check for retval of virJSONValueNewArray() virCommand: Make virCommandPassFDGetFDIndex fail if passed command is in error state storagepoolxml2argvtest: Avoid double free src/conf/virstorageobj.c | 6 ++++-- src/rpc/virnetserver.c | 8 ++++++-- src/util/vircommand.c | 3 +++ tests/storagepoolxml2argvtest.c | 1 - 4 files changed, 13 insertions(+), 5 deletions(-) -- 2.21.0
If virStoragePoolObjNew() fails to create new volume object list then virObjectUnref() is called and since refcounter is 1 then virStoragePoolObjDispose() is called which in turn calls virStoragePoolObjClearVols() which in turn dereferences obj->volumes. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 1d6c9d1937..1d5c88f50b 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -365,8 +365,10 @@ virStoragePoolObjDispose(void *opaque) if (!obj) return; - virStoragePoolObjClearVols(obj); - virObjectUnref(obj->volumes); + if (obj->volumes) { + virStoragePoolObjClearVols(obj); + virObjectUnref(obj->volumes); + } virStoragePoolDefFree(obj->def); virStoragePoolDefFree(obj->newDef); -- 2.21.0
On Tue, May 14, 2019 at 11:24:09AM +0200, Michal Privoznik wrote:
If virStoragePoolObjNew() fails to create new volume object list then virObjectUnref() is called and since refcounter is 1 then virStoragePoolObjDispose() is called which in turn calls virStoragePoolObjClearVols() which in turn dereferences obj->volumes.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 1d6c9d1937..1d5c88f50b 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -365,8 +365,10 @@ virStoragePoolObjDispose(void *opaque) if (!obj) return;
- virStoragePoolObjClearVols(obj); - virObjectUnref(obj->volumes); + if (obj->volumes) { + virStoragePoolObjClearVols(obj); + virObjectUnref(obj->volumes);
I think the check is better suited to live inside virStoragePoolObjClearVols as there are multiple callers to virStoragePoolObjClearVols, just to be on the safer side. Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetserver.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 83b871764f..4934dba967 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -629,7 +629,9 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv) goto error; } - services = virJSONValueNewArray(); + if (!(services = virJSONValueNewArray())) + goto error; + if (virJSONValueObjectAppend(object, "services", services) < 0) { virJSONValueFree(services); goto error; @@ -646,7 +648,9 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv) } } - clients = virJSONValueNewArray(); + if (!(clients = virJSONValueNewArray())) + goto error; + if (virJSONValueObjectAppend(object, "clients", clients) < 0) { virJSONValueFree(clients); goto error; -- 2.21.0
On Tue, May 14, 2019 at 11:24:10AM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetserver.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The idea of virCommand* APIs is that a possible error that occurred while constructing cmd line is kept in virCommand struct. If that's the case all subsequent calls to virCommand*() are NO-OPs or they return an error. Well, virCommandPassFDGetFDIndex() is not honoring that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 9e99697c55..8695c98d1b 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1034,6 +1034,9 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd) { size_t i = 0; + if (!cmd || cmd->has_error) + return -1; + while (i < cmd->npassfd) { if (cmd->passfd[i].fd == fd) return i; -- 2.21.0
On Tue, May 14, 2019 at 11:24:11AM +0200, Michal Privoznik wrote:
The idea of virCommand* APIs is that a possible error that occurred while constructing cmd line is kept in virCommand struct. If that's the case all subsequent calls to virCommand*() are NO-OPs or they return an error. Well, virCommandPassFDGetFDIndex() is not honoring that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
On Tue, May 14, 2019 at 11:24:11AM +0200, Michal Privoznik wrote:
The idea of virCommand* APIs is that a possible error that occurred while constructing cmd line is kept in virCommand struct. If that's the case all subsequent calls to virCommand*() are NO-OPs or they return an error. Well, virCommandPassFDGetFDIndex() is not honoring that.
This is the flaw that caused the crash I reported yesterday when running qemuxml2argvtest with ENOMEM simulation active.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 9e99697c55..8695c98d1b 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1034,6 +1034,9 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd) { size_t i = 0;
+ if (!cmd || cmd->has_error) + return -1; + while (i < cmd->npassfd) { if (cmd->passfd[i].fd == fd) return i;
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 :|
A double free may occur in testCompareXMLToArgvFiles() when @def is freed right after virStoragePoolObjNew() failed and the second time at cleanup label. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/storagepoolxml2argvtest.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index b7e32064af..0c01931946 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -39,7 +39,6 @@ testCompareXMLToArgvFiles(bool shouldFail, case VIR_STORAGE_POOL_NETFS: if (!(pool = virStoragePoolObjNew())) { VIR_TEST_DEBUG("pool type '%s' alloc pool obj fails\n", defTypeStr); - virStoragePoolDefFree(def); goto cleanup; } virStoragePoolObjSetDef(pool, def); -- 2.21.0
On Tue, May 14, 2019 at 11:24:12AM +0200, Michal Privoznik wrote:
A double free may occur in testCompareXMLToArgvFiles() when @def is freed right after virStoragePoolObjNew() failed and the second time at cleanup label.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Michal Privoznik