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@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@xxxxxxxxxx> 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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list