[Libvir] Fix buffer overflow in dumping XML
The new bufferContentAndFree() method used for the QEMU daemon rellocs the buffer size down to release memory held by the buffer which was never used for any data. Unfortunately it reallocs it 1 byte too small, so later uses of strlen()/strcpy() either magically work, or randomly append gargage or crash the daemon depending on the phase of the moon :-) Re-allocing the buffer to relase a few bytes memory isn't really an optimization since the caller is going to free the entire block a very short while later, so this patch simply removes the realloc call. As an aside, the virBuffer functions in src/xml.c and the buffer functions in qemud/buf.c are both flawed wrt to the way they call the Grow method. The method expects the len parameter to be extra bytes needed, but several of the callers pass in the total desired length, so it allocates too much memory. There are various other non-fatal flaws which need to be cleaned up in this code, but the attached patch just focuses on the current fatal buffer overflow for now. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Wed, Mar 21, 2007 at 03:09:09PM +0000, Daniel P. Berrange wrote:
The new bufferContentAndFree() method used for the QEMU daemon rellocs the buffer size down to release memory held by the buffer which was never used for any data. Unfortunately it reallocs it 1 byte too small, so later uses of strlen()/strcpy() either magically work, or randomly append gargage or crash the daemon depending on the phase of the moon :-) Re-allocing the buffer to relase a few bytes memory isn't really an optimization since the caller is going to free the entire block a very short while later, so this patch simply removes the realloc call.
Okay, please commit :-)
As an aside, the virBuffer functions in src/xml.c and the buffer functions in qemud/buf.c are both flawed wrt to the way they call the Grow method. The method expects the len parameter to be extra bytes needed, but several of the callers pass in the total desired length, so it allocates too much memory. There are various other non-fatal flaws which need to be cleaned up in this code, but the attached patch just focuses on the current fatal buffer overflow for now.
Okay, I fixed the problems, commited in CVS, I also clarified the documentationof those routines. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel P. Berrange wrote:
The new bufferContentAndFree() method used for the QEMU daemon rellocs the buffer size down to release memory held by the buffer which was never used for any data. Unfortunately it reallocs it 1 byte too small, so later uses of strlen()/strcpy() either magically work, or randomly append gargage or crash the daemon depending on the phase of the moon :-) Re-allocing the buffer to relase a few bytes memory isn't really an optimization since the caller is going to free the entire block a very short while later, so this patch simply removes the realloc call.
Ooops - good call. Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones