[libvirt] [PATCH] qemu: avoid pass null pointer as an argument
In fact, 'pos' is always -1, this reason is because qemuProcessStart function assigns -1 to 'pos' variable then call qemuProcessWaitForMonitor, meanwhile, qemuProcessAttach function also call qemuProcessWaitForMonitor and directly pass -1 as an argument, so if (pos != -1) statement can't been run for ever, it also means we can't allocate memory to 'buf' variable, that is, 'buf' is a initial value NULL, however, the function qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf)) will be called on 'cleanup' section, null pointer passed as an argument. * src/qemu/qemu_process.c: avoid null pointer passed as an argument to a 'nonnull' parameter. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/qemu/qemu_process.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b0d2149..570992d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1189,6 +1189,11 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, goto closelog; } + if (VIR_ALLOC_N(buf, buf_size) < 0) { + virReportOOMError(); + return -1; + } + VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name); if (qemuConnectMonitor(driver, vm) < 0) { goto cleanup; -- 1.7.5.1
On 08/04/2011 09:31 AM, Alex Jia wrote:
In fact, 'pos' is always -1, this reason is because qemuProcessStart function assigns -1 to 'pos' variable then call qemuProcessWaitForMonitor,
Actually, qemuProcessStart always calls qemuProcessWaitForMonitor with non-negative pos (pos was set by lseek()ing to the end of the log file).
meanwhile, qemuProcessAttach function also call qemuProcessWaitForMonitor and directly pass -1 as an argument, so if (pos != -1) statement can't been run for ever,
Rather, this merely means that the if (pos != -1) statement wasn't run when called by qemuProcessAttach.
it also means we can't allocate memory to 'buf' variable, that is, 'buf' is a initial value NULL, however, the function qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf)) will be called on 'cleanup' section, null pointer passed as an argument.
* src/qemu/qemu_process.c: avoid null pointer passed as an argument to a 'nonnull' parameter.
We definitely have a bug here, but this is not the right fix. The bug is that the cleanup: label is trying to read from logfd if the vm crashed, without having opened logfd in the qemuProcessAttach case. I think the more appropriate patch is this: diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 8508ff6..1eea45f 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1214,7 +1214,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, cleanup: virHashFree(paths); - if (kill(vm->pid, 0) == -1 && errno == ESRCH) { + if (pos != -1 && kill(vm->pid, 0) == -1 && errno == ESRCH) { /* VM is dead, any other error raised in the interim is probably * not as important as the qemu cmdline output */ qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf)); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/04/2011 02:07 PM, Eric Blake wrote:
We definitely have a bug here, but this is not the right fix. The bug is that the cleanup: label is trying to read from logfd if the vm crashed, without having opened logfd in the qemuProcessAttach case.
I think the more appropriate patch is this:
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 8508ff6..1eea45f 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1214,7 +1214,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, cleanup: virHashFree(paths);
- if (kill(vm->pid, 0) == -1 && errno == ESRCH) { + if (pos != -1 && kill(vm->pid, 0) == -1 && errno == ESRCH) { /* VM is dead, any other error raised in the interim is probably * not as important as the qemu cmdline output */ qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf));
Also squash this in - no need to free buf twice. diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 1eea45f..a37f709 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1221,16 +1221,14 @@ cleanup: qemuReportError(VIR_ERR_INTERNAL_ERROR, _("process exited while connecting to monitor: %s"), buf); ret = -1; } closelog: - VIR_FREE(buf); - if (VIR_CLOSE(logfd) < 0) { char ebuf[1024]; VIR_WARN("Unable to close logfile: %s", virStrerror(errno, ebuf, sizeof ebuf)); } VIR_FREE(buf); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/05/2011 04:17 AM, Eric Blake wrote:
On 08/04/2011 02:07 PM, Eric Blake wrote:
We definitely have a bug here, but this is not the right fix. The bug is that the cleanup: label is trying to read from logfd if the vm crashed, without having opened logfd in the qemuProcessAttach case.
I think the more appropriate patch is this:
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 8508ff6..1eea45f 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1214,7 +1214,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, cleanup: virHashFree(paths);
- if (kill(vm->pid, 0) == -1 && errno == ESRCH) { + if (pos != -1 && kill(vm->pid, 0) == -1 && errno == ESRCH) { /* VM is dead, any other error raised in the interim is probably * not as important as the qemu cmdline output */ qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf));
Also squash this in - no need to free buf twice.
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 1eea45f..a37f709 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1221,16 +1221,14 @@ cleanup: qemuReportError(VIR_ERR_INTERNAL_ERROR, _("process exited while connecting to monitor: %s"), buf); ret = -1; }
closelog: - VIR_FREE(buf); - if (VIR_CLOSE(logfd) < 0) { char ebuf[1024]; VIR_WARN("Unable to close logfile: %s", virStrerror(errno, ebuf, sizeof ebuf)); }
VIR_FREE(buf);
Right, this is a double free, Eric, Hasn't Coverity found this issue? Thanks, Alex
On 08/04/2011 07:00 PM, Alex Jia wrote:
closelog: - VIR_FREE(buf); - if (VIR_CLOSE(logfd) < 0) { char ebuf[1024]; VIR_WARN("Unable to close logfile: %s", virStrerror(errno, ebuf, sizeof ebuf)); }
VIR_FREE(buf);
Right, this is a double free, Eric, Hasn't Coverity found this issue?
It's not a double free, since the first VIR_FREE sets buf to NULL, making the second one a no-op, which is why Coverity won't warn about it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/05/2011 04:07 AM, Eric Blake wrote:
On 08/04/2011 09:31 AM, Alex Jia wrote:
In fact, 'pos' is always -1, this reason is because qemuProcessStart function assigns -1 to 'pos' variable then call qemuProcessWaitForMonitor,
Actually, qemuProcessStart always calls qemuProcessWaitForMonitor with non-negative pos (pos was set by lseek()ing to the end of the log file).
meanwhile, qemuProcessAttach function also call qemuProcessWaitForMonitor and directly pass -1 as an argument, so if (pos != -1) statement can't been run for ever,
Rather, this merely means that the if (pos != -1) statement wasn't run when called by qemuProcessAttach.
it also means we can't allocate memory to 'buf' variable, that is, 'buf' is a initial value NULL, however, the function qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf)) will be called on 'cleanup' section, null pointer passed as an argument.
* src/qemu/qemu_process.c: avoid null pointer passed as an argument to a 'nonnull' parameter.
We definitely have a bug here, but this is not the right fix. The bug is that the cleanup: label is trying to read from logfd if the vm crashed, without having opened logfd in the qemuProcessAttach case.
I think the more appropriate patch is this:
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 8508ff6..1eea45f 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1214,7 +1214,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, cleanup: virHashFree(paths);
- if (kill(vm->pid, 0) == -1 && errno == ESRCH) { + if (pos != -1 && kill(vm->pid, 0) == -1 && errno == ESRCH) { /* VM is dead, any other error raised in the interim is probably * not as important as the qemu cmdline output */ qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf));
Agree, it indeed is a issue, I will check 'null pointer' issue with thiis fixed again, to avoid some warning from ccc-analyzer, if you set up this env, please also check it. Thanks, Alex
On 08/04/2011 07:10 PM, Alex Jia wrote:
We definitely have a bug here, but this is not the right fix. The bug is that the cleanup: label is trying to read from logfd if the vm crashed, without having opened logfd in the qemuProcessAttach case.
I think the more appropriate patch is this:
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 8508ff6..1eea45f 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1214,7 +1214,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, cleanup: virHashFree(paths);
- if (kill(vm->pid, 0) == -1 && errno == ESRCH) { + if (pos != -1 && kill(vm->pid, 0) == -1 && errno == ESRCH) { /* VM is dead, any other error raised in the interim is probably * not as important as the qemu cmdline output */ qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf));
Agree, it indeed is a issue, I will check 'null pointer' issue with thiis fixed again, to avoid some warning from ccc-analyzer, if you set up this env, please also check it.
With that, I've gone ahead and pushed this, which also included fixing the double VIR_FREE. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake