[libvirt] [PATCH] Eliminate large stack buffer in doTunnelSendAll
doTunnelSendAll function (used by QEMU migration) uses a 64k buffer on the stack, which could be problematic. This patch replaces that with a buffer from the heap. While in the neighborhood, this patch also improves error reporting in the case that saferead fails - previously, virStreamAbort() was called (resetting errno) before reporting the error. It's been changed to report the error first. --- src/qemu/qemu_driver.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bb3edde..463c716 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8399,19 +8399,28 @@ cleanup: } +#define TUNNEL_SEND_BUF_SIZE 65536 + static int doTunnelSendAll(virStreamPtr st, int sock) { - char buffer[65536]; - int nbytes = sizeof(buffer); + char *buffer; + int nbytes = TUNNEL_SEND_BUF_SIZE; + + if (VIR_ALLOC_N(buffer, TUNNEL_SEND_BUF_SIZE) < 0) { + virStreamAbort(st); + virReportOOMError(); + return -1; + } /* XXX should honour the 'resource' parameter here */ for (;;) { nbytes = saferead(sock, buffer, nbytes); if (nbytes < 0) { - virStreamAbort(st); virReportSystemError(errno, "%s", _("tunnelled migration failed to read from qemu")); + virStreamAbort(st); + VIR_FREE(buffer); return -1; } else if (nbytes == 0) @@ -8421,10 +8430,13 @@ static int doTunnelSendAll(virStreamPtr st, if (virStreamSend(st, buffer, nbytes) < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Failed to write migration data to remote libvirtd")); + VIR_FREE(buffer); return -1; } } + VIR_FREE(buffer); + if (virStreamFinish(st) < 0) /* virStreamFinish set the error for us */ return -1; -- 1.6.6.1
On 03/05/2010 02:30 PM, Laine Stump wrote:
doTunnelSendAll function (used by QEMU migration) uses a 64k buffer on the stack, which could be problematic. This patch replaces that with a buffer from the heap.
There's a lot more large stacks noted by Coverity, but this is a good start.
While in the neighborhood, this patch also improves error reporting in the case that saferead fails - previously, virStreamAbort() was called (resetting errno) before reporting the error. It's been changed to report the error first.
Yep.
- char buffer[65536]; - int nbytes = sizeof(buffer); + char *buffer; + int nbytes = TUNNEL_SEND_BUF_SIZE; + + if (VIR_ALLOC_N(buffer, TUNNEL_SEND_BUF_SIZE) < 0) { + virStreamAbort(st); + virReportOOMError();
Is this backwards?
+ return -1; + }
/* XXX should honour the 'resource' parameter here */ for (;;) { nbytes = saferead(sock, buffer, nbytes); if (nbytes < 0) { - virStreamAbort(st); virReportSystemError(errno, "%s", _("tunnelled migration failed to read from qemu")); + virStreamAbort(st); + VIR_FREE(buffer);
Especially given that you just reordered the virStreamAbort to come last here? If the above was not a mistake, then ACK to the patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/05/2010 04:40 PM, Eric Blake wrote:
On 03/05/2010 02:30 PM, Laine Stump wrote:
doTunnelSendAll function (used by QEMU migration) uses a 64k buffer on the stack, which could be problematic. This patch replaces that with a buffer from the heap.
There's a lot more large stacks noted by Coverity, but this is a good start.
Heh. Yeah, this one happened to catch my eye last week while I was in this file, and I couldn't let it stand ;-)
While in the neighborhood, this patch also improves error reporting in the case that saferead fails - previously, virStreamAbort() was called (resetting errno) before reporting the error. It's been changed to report the error first.
Yep.
- char buffer[65536]; - int nbytes = sizeof(buffer); + char *buffer; + int nbytes = TUNNEL_SEND_BUF_SIZE; + + if (VIR_ALLOC_N(buffer, TUNNEL_SEND_BUF_SIZE)< 0) { + virStreamAbort(st); + virReportOOMError();
Is this backwards?
Order isn't as important in this case as the other (the one I changed), since virReportOOMError() doesn't need to look at errno. But I agree it would be more consistent to do them both in the same order. I just resubmitted (and forgot to set the In-Reply-To header so it would show up in this thread. #&%$!)
+ return -1; + }
/* XXX should honour the 'resource' parameter here */ for (;;) { nbytes = saferead(sock, buffer, nbytes); if (nbytes< 0) { - virStreamAbort(st); virReportSystemError(errno, "%s", _("tunnelled migration failed to read from qemu")); + virStreamAbort(st); + VIR_FREE(buffer);
Especially given that you just reordered the virStreamAbort to come last here? If the above was not a mistake, then ACK to the patch.
participants (2)
-
Eric Blake -
Laine Stump