[libvirt] [PATCH] vbox: fix segfault on virsh dumpxml with the existence of USB filters
A USB filter is stored in a hostdev. The original code doesn't allocate hostdev->info that is expected to be allocated with hostdev. So use virDomainHostdevDefAlloc() to allocate both as we expect. Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com> --- src/vbox/vbox_tmpl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 3807a6d..f4cd5cf 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3333,7 +3333,8 @@ sharedFoldersCleanup: deviceFilter->vtbl->GetActive(deviceFilter, &active); if (active) { - if (VIR_ALLOC(def->hostdevs[USBFilterCount]) >= 0) { + def->hostdevs[USBFilterCount] = virDomainHostdevDefAlloc(); + if (def->hostdevs[USBFilterCount]) { PRUnichar *vendorIdUtf16 = NULL; char *vendorIdUtf8 = NULL; unsigned vendorId = 0; -- 1.8.4
On 11/18/2013 05:39 PM, Ryota Ozaki wrote:
A USB filter is stored in a hostdev. The original code doesn't allocate hostdev->info that is expected to be allocated with hostdev. So use virDomainHostdevDefAlloc() to allocate both as we expect.
Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com> --- src/vbox/vbox_tmpl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 3807a6d..f4cd5cf 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3333,7 +3333,8 @@ sharedFoldersCleanup:
deviceFilter->vtbl->GetActive(deviceFilter, &active); if (active) { - if (VIR_ALLOC(def->hostdevs[USBFilterCount]) >= 0) { + def->hostdevs[USBFilterCount] = virDomainHostdevDefAlloc(); + if (def->hostdevs[USBFilterCount]) { PRUnichar *vendorIdUtf16 = NULL; char *vendorIdUtf8 = NULL; unsigned vendorId = 0;
ACK on this fix, which I pushed. But just taking a quick look at that small chunk of the gigantic function it's part of (vboxDomainGetXMLDesc), I saw two other problems that need fixing. Since you seem to have an interest in a properly working vbox driver, you might want to look into making patches to: 1) properly handle out of memory errors (and other errors - there is absolutely no error handling in that function). 2) Fix this apparently incorrect comparison just above the current patch: for (i = 0; (USBFilterCount < def->nhostdevs) || (i < deviceFilters.count); i++) { (I'm pretty sure that "||" needs to be "&&", but don't want to patch code that I can't test).
On Tue, Nov 19, 2013 at 11:29 PM, Laine Stump <laine@laine.org> wrote:
On 11/18/2013 05:39 PM, Ryota Ozaki wrote:
A USB filter is stored in a hostdev. The original code doesn't allocate hostdev->info that is expected to be allocated with hostdev. So use virDomainHostdevDefAlloc() to allocate both as we expect.
Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com> --- src/vbox/vbox_tmpl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 3807a6d..f4cd5cf 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3333,7 +3333,8 @@ sharedFoldersCleanup:
deviceFilter->vtbl->GetActive(deviceFilter, &active); if (active) { - if (VIR_ALLOC(def->hostdevs[USBFilterCount]) >= 0) { + def->hostdevs[USBFilterCount] = virDomainHostdevDefAlloc(); + if (def->hostdevs[USBFilterCount]) { PRUnichar *vendorIdUtf16 = NULL; char *vendorIdUtf8 = NULL; unsigned vendorId = 0;
ACK on this fix, which I pushed.
Thanks Laine!
But just taking a quick look at that small chunk of the gigantic function it's part of (vboxDomainGetXMLDesc), I saw two other problems that need fixing. Since you seem to have an interest in a properly working vbox driver, you might want to look into making patches to:
Yes, I'm using the vbox driver for testing on Mac OS X. Actually the fixed bug was found when I was working on API v4.3 support.
1) properly handle out of memory errors (and other errors - there is absolutely no error handling in that function).
2) Fix this apparently incorrect comparison just above the current patch:
for (i = 0; (USBFilterCount < def->nhostdevs) || (i < deviceFilters.count); i++) {
(I'm pretty sure that "||" needs to be "&&", but don't want to patch code that I can't test).
Thank you for pointing out. I'm queuing the fixes and will send patches after I finish API v4.3 support. ozaki-r
participants (2)
-
Laine Stump -
Ryota Ozaki