From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com (mx1.redhat.com [172.16.48.31]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n0FFFJSk007161 for ; Thu, 15 Jan 2009 10:15:19 -0500 Received: from iamta51.mxsweep.com (mail151.ix.emailantidote.com [89.167.219.151]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n0FFEte0022815 for ; Thu, 15 Jan 2009 10:14:59 -0500 Message-ID: <496F4F1B.1050706@draigBrady.com> Date: Thu, 15 Jan 2009 14:58:35 +0000 From: =?ISO-8859-1?Q?P=E1draig_Brady?= MIME-Version: 1.0 To: Robert Krakora References: <496F18C4.9020009@draigBrady.com> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 8bit Cc: video4linux-list@redhat.com Subject: Re: [PATCH 2.6.27.8 1/1] em28xx: Fix audio URB transfer buffer memory leak and race condition/corruption of capture pointer List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: video4linux-list-bounces@redhat.com Errors-To: video4linux-list-bounces@redhat.com List-ID: Robert Krakora wrote: > On Thu, Jan 15, 2009 at 6:06 AM, Pádraig Brady wrote: >> Robert Krakora wrote: >>> em28xx: Fix audio URB transfer buffer memory leak and race >>> condition/corruption of capture pointer >>> >>> >>> Signed-off-by: Robert V. Krakora >>> >>> diff -r 6896782d783d linux/drivers/media/video/em28xx/em28xx-audio.c >>> --- a/linux/drivers/media/video/em28xx/em28xx-audio.c Wed Jan 14 >>> 10:06:12 2009 -0200 >>> +++ b/linux/drivers/media/video/em28xx/em28xx-audio.c Wed Jan 14 >>> 12:47:00 2009 -0500 >>> @@ -62,11 +62,20 @@ >>> int i; >>> >>> dprintk("Stopping isoc\n"); >>> - for (i = 0; i < EM28XX_AUDIO_BUFS; i++) { >>> - usb_unlink_urb(dev->adev.urb[i]); >>> - usb_free_urb(dev->adev.urb[i]); >>> - dev->adev.urb[i] = NULL; >>> - } >>> + for (i = 0; i < EM28XX_AUDIO_BUFS; i++) { >>> + usb_unlink_urb(dev->adev.urb[i]); >>> + usb_free_urb(dev->adev.urb[i]); >>> + dev->adev.urb[i] = NULL; >>> + if (dev->adev.urb[i]) { >>> + usb_unlink_urb(dev->adev.urb[i]); >>> + usb_free_urb(dev->adev.urb[i]); >>> + dev->adev.urb[i] = NULL; >>> + } >>> + if (dev->adev.transfer_buffer) { >>> + kfree(dev->adev.transfer_buffer[i]); >>> + dev->adev.transfer_buffer[i] = NULL; >>> + } >>> + } >>> >>> return 0; >>> } >> That looks a bit incorrect. I fixed this last week in Markus' >> repository, as I thought the leak was specific to that tree: >> http://mcentral.de/hg/~mrec/em28xx-new/diff/1cfd9010a552/em28xx-audio.c >> > > I fail to see what looks incorrect about testing for NULL pointers > before freeing. That's redundant/inefficient. kfree(NULL) is fine. > I did have a bug where I left the index number off of > the transfer buffer array which Devin kindly pointed out yesterday. I was referring to that yes. I was also referring to the fact you have 2 calls to free the URBs. I suggest you just do what I did in my patch. I've tested it well. Without it I was leaking 48KiB every time VLC changed channel. cheers, Pádraig. -- video4linux-list mailing list Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe https://www.redhat.com/mailman/listinfo/video4linux-list