[libvirt] [PATCH 1/2] qemuProcessReadLog: Fix memcpy arguments

[Ján Tomko]

s/memcpy/memmove/ in the summary

On Mon, Jan 18, 2016 at 11:39:34AM +0100, Michal Privoznik wrote:
> So I can observe this crasher that with freshly started daemon
> (and virtlogd enabled) I am trying to startup a domain that
> immediately dies (because it's said to use huge pages but I
> haven't allocated a single one in the pool).

> But what's
> interesting is that the daemon crashes too.

Redundant sentence.

> Hardly reproducible
> with -O0 or under valgrind.

> But I just got lucky:

Also redundant.

> 
> ==20469== Invalid write of size 8
> ==20469==    at 0x4C2E99B: memcpy at GLIBC_2.2.5 (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==20469==    by 0x217EDD07: qemuProcessReadLog (qemu_process.c:1670)
> ==20469==    by 0x217EDE1D: qemuProcessReportLogError (qemu_process.c:1696)
> ==20469==    by 0x217EE8C1: qemuProcessWaitForMonitor (qemu_process.c:1957)
> ==20469==    by 0x217F6636: qemuProcessLaunch (qemu_process.c:4955)
> ==20469==    by 0x217F71A4: qemuProcessStart (qemu_process.c:5152)
> ==20469==    by 0x21846582: qemuDomainObjStart (qemu_driver.c:7396)
> ==20469==    by 0x218467DE: qemuDomainCreateWithFlags (qemu_driver.c:7450)
> ==20469==    by 0x21846845: qemuDomainCreate (qemu_driver.c:7468)
> ==20469==    by 0x5611CD0: virDomainCreate (libvirt-domain.c:6753)
> ==20469==    by 0x125D9A: remoteDispatchDomainCreate (remote_dispatch.h:3613)
> ==20469==    by 0x125CB7: remoteDispatchDomainCreateHelper (remote_dispatch.h:3589)
> ==20469==  Address 0x27a52ad0 is 0 bytes after a block of size 5,584 alloc'd
> ==20469==    at 0x4C29F80: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==20469==    by 0x9B8D1DB: xdr_string (in /lib64/libc-2.21.so)
> ==20469==    by 0x563B39C: xdr_virLogManagerProtocolNonNullString (log_protocol.c:24)
> ==20469==    by 0x563B6B7: xdr_virLogManagerProtocolDomainReadLogFileRet (log_protocol.c:123)
> ==20469==    by 0x164B34: virNetMessageDecodePayload (virnetmessage.c:407)
> ==20469==    by 0x5682360: virNetClientProgramCall (virnetclientprogram.c:379)
> ==20469==    by 0x563B30E: virLogManagerDomainReadLogFile (log_manager.c:272)
> ==20469==    by 0x217CD613: qemuDomainLogContextRead (qemu_domain.c:2485)
> ==20469==    by 0x217EDC76: qemuProcessReadLog (qemu_process.c:1660)
> ==20469==    by 0x217EDE1D: qemuProcessReportLogError (qemu_process.c:1696)
> ==20469==    by 0x217EE8C1: qemuProcessWaitForMonitor (qemu_process.c:1957)
> ==20469==    by 0x217F6636: qemuProcessLaunch (qemu_process.c:4955)
> 

The backtrace matches this report by Luyao:
https://www.redhat.com/archives/libvir-list/2015-December/msg00329.html

> This points to memmove() in qemuProcessReadLog().

> And in fact
> after few moments and a couple of papers (to debug the code) I
> can observe the bug.

This can be dropped too.

> Imagine we just read the following string
> from qemu:
> 
> "abc\n2016-01-18T09:40:44.022744Z qemu-system-x86_64: Error\n"
> 
> After the first pass of the while() loop in the
> qemuProcessReadLog() @buf still points to the beginning of the
> string, @filter_next points to the beginning of the second line.
> So we start second iteration because there is yet another newline
> character at the end. In this iteration @eol points to it
> actually. Now, the control gets inside true branch of if(). Just
> to remind you:
> 
> got = 58
> filter_next = buf + 5,
> eol = buf + 58.
> 
> Therefore skip = 54 which is correct. The message we want to skip
> is 54 bytes long. However:
> 
> memmove(filter_next, eol + 1, (got - skip) +1);
> 
> which is
> 
> memmove(filter_next, eol + 1, 5)
> 
> is obviously wrong as there is only one byte we can access, not 5!
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>

Introduced by v1.2.21-150-g69b0992

> ---
>  src/qemu/qemu_process.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK if you strip your life story from the commit message.

Jan

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 05cbda2..f85afd5 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1667,7 +1667,7 @@ qemuProcessReadLog(qemuDomainLogContextPtr logCtxt, char **msg)
>          if (virLogProbablyLogMessage(filter_next) ||
>              STRPREFIX(filter_next, "char device redirected to")) {
>              size_t skip = (eol + 1) - filter_next;
> -            memmove(filter_next, eol + 1, (got - skip) + 1);
> +            memmove(filter_next, eol + 1, buf + got - eol);
>              got -= skip;
>          } else {
>              filter_next = eol + 1;
> -- 
> 2.4.10
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160118/bfdbfeee/attachment-0001.sig>