* Re: [Qemu-devel] [PATCH] usb: ohci avoid multiple eof timers
2016-02-16 10:04 [Qemu-devel] [PATCH] usb: ohci avoid multiple eof timers P J P
@ 2016-02-16 10:56 ` Laurent Vivier
2016-02-16 14:17 ` Gerd Hoffmann
1 sibling, 0 replies; 4+ messages in thread
From: Laurent Vivier @ 2016-02-16 10:56 UTC (permalink / raw)
To: P J P, Qemu Developers; +Cc: Zuozhi Fzz, Gerd Hoffmann, Prasad J Pandit
On 16/02/2016 11:04, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> When transitioning an OHCI controller to the OHCI_USB_OPERATIONAL
> state, it creates an eof timer object in 'ohci_bus_start'.
> It does not check if one already exists. This results in memory
> leakage and null dereference issue. Add a check to avoid it.
>
> Reported-by: Zuozhi Fzz <zuozhi.fzz@alibaba-inc.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> ---
> hw/usb/hcd-ohci.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 7d65818..15f0b44 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -1331,11 +1331,11 @@ static void ohci_frame_boundary(void *opaque)
> */
> static int ohci_bus_start(OHCIState *ohci)
> {
> - ohci->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> - ohci_frame_boundary,
> - ohci);
> -
> - if (ohci->eof_timer == NULL) {
> + if (!ohci->eof_timer) {
> + ohci->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> + ohci_frame_boundary, ohci);
> + }
> + if (!ohci->eof_timer) {
> trace_usb_ohci_bus_eof_timer_failed(ohci->name);
> ohci_die(ohci);
> return 0;
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] usb: ohci avoid multiple eof timers
2016-02-16 10:04 [Qemu-devel] [PATCH] usb: ohci avoid multiple eof timers P J P
2016-02-16 10:56 ` Laurent Vivier
@ 2016-02-16 14:17 ` Gerd Hoffmann
2016-02-16 17:32 ` P J P
1 sibling, 1 reply; 4+ messages in thread
From: Gerd Hoffmann @ 2016-02-16 14:17 UTC (permalink / raw)
To: P J P; +Cc: Zuozhi Fzz, Qemu Developers, Prasad J Pandit
[-- Attachment #1: Type: text/plain, Size: 472 bytes --]
On Di, 2016-02-16 at 15:34 +0530, P J P wrote:
> When transitioning an OHCI controller to the OHCI_USB_OPERATIONAL
> state, it creates an eof timer object in 'ohci_bus_start'.
> It does not check if one already exists. This results in memory
> leakage and null dereference issue. Add a check to avoid it.
Well, allocating and deallocating the timer all the time isn't a great
idea in the first place. Can you try the attached patch instead?
thanks,
Gerd
[-- Attachment #2: 0001-ohci-timer-fix.patch --]
[-- Type: text/x-patch, Size: 2330 bytes --]
From d1b07becc481e09225cfe905ec357807ae07f095 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 16 Feb 2016 15:15:04 +0100
Subject: [PATCH] ohci timer fix
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/usb/hcd-ohci.c | 31 +++++--------------------------
1 file changed, 5 insertions(+), 26 deletions(-)
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index bed55dd..3d1270d 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1347,16 +1347,6 @@ static void ohci_frame_boundary(void *opaque)
*/
static int ohci_bus_start(OHCIState *ohci)
{
- ohci->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
- ohci_frame_boundary,
- ohci);
-
- if (ohci->eof_timer == NULL) {
- trace_usb_ohci_bus_eof_timer_failed(ohci->name);
- ohci_die(ohci);
- return 0;
- }
-
trace_usb_ohci_start(ohci->name);
/* Delay the first SOF event by one frame time as
@@ -1373,11 +1363,7 @@ static int ohci_bus_start(OHCIState *ohci)
static void ohci_bus_stop(OHCIState *ohci)
{
trace_usb_ohci_stop(ohci->name);
- if (ohci->eof_timer) {
- timer_del(ohci->eof_timer);
- timer_free(ohci->eof_timer);
- }
- ohci->eof_timer = NULL;
+ timer_del(ohci->eof_timer);
}
/* Sets a flag in a port status register but only set it if the port is
@@ -1907,6 +1893,9 @@ static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
usb_packet_init(&ohci->usb_packet);
ohci->async_td = 0;
+
+ ohci->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ ohci_frame_boundary, ohci);
}
#define TYPE_PCI_OHCI "pci-ohci"
@@ -2041,23 +2030,13 @@ static bool ohci_eof_timer_needed(void *opaque)
{
OHCIState *ohci = opaque;
- return ohci->eof_timer != NULL;
-}
-
-static int ohci_eof_timer_pre_load(void *opaque)
-{
- OHCIState *ohci = opaque;
-
- ohci_bus_start(ohci);
-
- return 0;
+ return timer_pending(ohci->eof_timer);
}
static const VMStateDescription vmstate_ohci_eof_timer = {
.name = "ohci-core/eof-timer",
.version_id = 1,
.minimum_version_id = 1,
- .pre_load = ohci_eof_timer_pre_load,
.needed = ohci_eof_timer_needed,
.fields = (VMStateField[]) {
VMSTATE_TIMER_PTR(eof_timer, OHCIState),
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread