[libvirt] [PATCH 0/4] Couple of recent things from Coverity
Some patches from recent changes for things Coverity has complained about John Ferlan (4): qemu: Remove unnecessary check in qemuMonitorJSONGetJobInfoOne test: Return early in testQueryJobs util: Avoid possible error in virCommandMassClose tests: Avoid possible error in testExecRestart src/qemu/qemu_monitor_json.c | 3 --- src/util/vircommand.c | 2 +- tests/qemumonitorjsontest.c | 3 +++ tests/virnetdaemontest.c | 9 +++++---- 4 files changed, 9 insertions(+), 8 deletions(-) -- 2.20.1
It's already dereffed in the initialization and shouldn't be NULL unless virJSONValueArraySize after a virJSONValueObjectGetArray could return a NULL data entry. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index f19dced014..d38b2f2cbe 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9133,9 +9133,6 @@ qemuMonitorJSONGetJobInfoOne(virJSONValuePtr data) VIR_AUTOPTR(qemuMonitorJobInfo) job = NULL; qemuMonitorJobInfoPtr ret = NULL; - if (!data) - return NULL; - if (VIR_ALLOC(job) < 0) return NULL; -- 2.20.1
Avoid the chance that qemuMonitorTestNewSimple could return NULL Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemumonitorjsontest.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 8eb9d2e01b..bf89f49aca 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2959,6 +2959,9 @@ testQueryJobs(const void *opaque) size_t i; int ret = -1; + if (!test) + return -1; + if (virAsprintf(&filenameJSON, abs_srcdir "/qemumonitorjsondata/query-jobs-%s.json", data->name) < 0 || -- 2.20.1
Avoid the chance that sysconf(_SC_OPEN_MAX) returns -1 and thus would cause virBitmapNew would attempt to allocate a very large bitmap. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/vircommand.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 4501c96bbf..cedbd01446 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -487,7 +487,7 @@ virCommandMassClose(virCommandPtr cmd, * Therefore we can safely allocate memory here (and transitively call * opendir/readdir) without a deadlock. */ - if (!(fds = virBitmapNew(openmax))) + if (openmax < 0 || !(fds = virBitmapNew(openmax))) return -1; # ifdef __linux__ -- 2.20.1
On Tue, Jul 23, 2019 at 09:58:10 -0400, John Ferlan wrote:
Avoid the chance that sysconf(_SC_OPEN_MAX) returns -1 and thus would cause virBitmapNew would attempt to allocate a very large bitmap.
Found by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/vircommand.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 4501c96bbf..cedbd01446 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -487,7 +487,7 @@ virCommandMassClose(virCommandPtr cmd, * Therefore we can safely allocate memory here (and transitively call * opendir/readdir) without a deadlock. */
- if (!(fds = virBitmapNew(openmax))) + if (openmax < 0 || !(fds = virBitmapNew(openmax))) return -1;
This would not report an libvirt error, but all other code paths do so.
# ifdef __linux__ -- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
If the dup2 fails, then we aren't going to get the correct result. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virnetdaemontest.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c index eae630a8f1..d0e9f241e3 100644 --- a/tests/virnetdaemontest.c +++ b/tests/virnetdaemontest.c @@ -287,10 +287,11 @@ static int testExecRestart(const void *opaque) * fds 100->103 for something else, which is probably * fairly reasonable in general */ - dup2(fdserver[0], 100); - dup2(fdserver[1], 101); - dup2(fdclient[0], 102); - dup2(fdclient[1], 103); + if (dup2(fdserver[0], 100) < 0 || + dup2(fdserver[1], 101) < 0 || + dup2(fdclient[0], 102) < 0 || + dup2(fdclient[1], 103) < 0) + goto cleanup; if (virAsprintf(&infile, "%s/virnetdaemondata/input-data-%s.json", abs_srcdir, data->jsonfile) < 0) -- 2.20.1
On Tue, Jul 23, 2019 at 09:58:11 -0400, John Ferlan wrote:
If the dup2 fails, then we aren't going to get the correct result.
Found by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virnetdaemontest.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c index eae630a8f1..d0e9f241e3 100644 --- a/tests/virnetdaemontest.c +++ b/tests/virnetdaemontest.c @@ -287,10 +287,11 @@ static int testExecRestart(const void *opaque) * fds 100->103 for something else, which is probably * fairly reasonable in general */ - dup2(fdserver[0], 100); - dup2(fdserver[1], 101); - dup2(fdclient[0], 102); - dup2(fdclient[1], 103); + if (dup2(fdserver[0], 100) < 0 || + dup2(fdserver[1], 101) < 0 || + dup2(fdclient[0], 102) < 0 || + dup2(fdclient[1], 103) < 0) + goto cleanup;
This will not print any debug message if we hit this.
if (virAsprintf(&infile, "%s/virnetdaemondata/input-data-%s.json", abs_srcdir, data->jsonfile) < 0) -- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jul 23, 2019 at 09:58:07 -0400, John Ferlan wrote:
Some patches from recent changes for things Coverity has complained about
John Ferlan (4): qemu: Remove unnecessary check in qemuMonitorJSONGetJobInfoOne test: Return early in testQueryJobs util: Avoid possible error in virCommandMassClose tests: Avoid possible error in testExecRestart
ACK series if you properly deal with error reporting in patches 3 and 4
participants (2)
-
John Ferlan -
Peter Krempa