Hi,
On Sun, Feb 03, 2013 at 06:18:57PM +0100, Guido Günther wrote:
> Hi Eric,
> On Tue, Jan 29, 2013 at 02:21:30PM -0700, Eric Blake wrote:
> > On 01/29/2013 01:22 PM, Guido Günther wrote:
> > > Hi,
> > > On Mon, Jan 28, 2013 at 07:35:38PM +0100, Peter Krempa wrote:
> > >> When reading and dispatching of a message failed the message was freed
> > >> but wasn't removed from the message queue.
> > >>
> > >> After that when the connection was about to be closed the pointer for
> > >> the message was still present in the queue and it was passed to
> > >> virNetMessageFree which tried to call the callback function from an
> > >> uninitialized pointer.
> > >
> > > Debian stable is shipping 0.8.2. I checked and it seems this version
> > > isn't affected siince we properly remove the message from the queue
> > > before looking at it in daemon/libvirtd.c. I'd be great if somebody
> > > could double check though!
> >
> > 0.8.2 predates the RPC rewrite, and I concur with your assessment that
> > back then, the code was _always_ clearing the queue:
> >
> > v0.8.2:daemon/libvirtd.c:qemudDispatchClientRead():
> >
> > /* Grab the completed message */
> > struct qemud_client_message *msg =
> > qemudClientMessageQueueServe(&client->rx);
> > struct qemud_client_filter *filter;
> >
> > /* Decode the header so we can use it for routing decisions */
> > if (remoteDecodeClientMessageHeader(msg) < 0) {
> > VIR_FREE(msg);
> > qemudDispatchClientFailure(client);
> > }
> >
> > However, it does look like there might be a missing 'return' statement
> > after that error is reported, especially given that the next error
> > reporting a few lines later does an early return.
>
> Thanks for double checking. It indeed looks like there's a return
> missing (cc:'ing the Debian bugreport to make this information permanent
> there too).
Sorry for the delay but attached patch would fix the issue in Squeeze.
I didn't reference the CVE since it's a kind of different problem but
could do so of course if needed.
Cheers,
-- Guido
diff -Nru libvirt-0.8.3/debian/changelog libvirt-0.8.3/debian/changelog
--- libvirt-0.8.3/debian/changelog 2011-07-16 20:40:00.000000000 +0200
+++ libvirt-0.8.3/debian/changelog 2013-02-22 20:32:53.000000000 +0100
@@ -1,3 +1,9 @@
+libvirt (0.8.3-5+squeeze3) stable-security; urgency=low
+
+ * [0bbbca1] Add missing return on error path (Closes: #699224)
+
+ -- Guido Günther <a...@sigxcpu.org> Fri, 22 Feb 2013 20:32:53 +0100
+
libvirt (0.8.3-5+squeeze2) stable-security; urgency=low
* [ac67c93] CVE-2011-1486: Make error reporting in libvirtd thread safe
diff -Nru
libvirt-0.8.3/debian/patches/0016-Add-missing-return-on-error-path.patch
libvirt-0.8.3/debian/patches/0016-Add-missing-return-on-error-path.patch
--- libvirt-0.8.3/debian/patches/0016-Add-missing-return-on-error-path.patch
1970-01-01 01:00:00.000000000 +0100
+++ libvirt-0.8.3/debian/patches/0016-Add-missing-return-on-error-path.patch
2013-02-22 20:06:37.000000000 +0100
@@ -0,0 +1,20 @@
+From: =?UTF-8?q?Guido=20G=C3=BCnther?= <a...@sigxcpu.org>
+Date: Fri, 22 Feb 2013 20:06:25 +0100
+Subject: Add missing return on error path
+
+---
+ daemon/libvirtd.c | 1 +
+ 1 file changed, 1 insertion(+)
+
+diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
+index 711360b..9b88aac 100644
+--- a/daemon/libvirtd.c
++++ b/daemon/libvirtd.c
+@@ -1820,6 +1820,7 @@ readmore:
+ if (remoteDecodeClientMessageHeader(msg) < 0) {
+ VIR_FREE(msg);
+ qemudDispatchClientFailure(client);
++ return;
+ }
+
+ /* Check if any filters match this message */
diff -Nru libvirt-0.8.3/debian/patches/series
libvirt-0.8.3/debian/patches/series
--- libvirt-0.8.3/debian/patches/series 2011-07-16 20:40:00.000000000 +0200
+++ libvirt-0.8.3/debian/patches/series 2013-02-22 20:06:37.000000000 +0100
@@ -13,3 +13,4 @@
security/0013-Add-missing-checks-for-read-only-connections.patch
security/0014-Make-error-reporting-in-libvirtd-thread-safe.patch
security/0015-Fix-integer-overflow-in-VirDomainGetVcpus.patch
+0016-Add-missing-return-on-error-path.patch
