Re: [PATCH 2.6.27.8 1/1] em28xx: Fix audio URB transfer buffer memory leak and race condition/corruption of capture pointer

["Pádraig Brady" <P@draigBrady.com>]

Robert Krakora wrote:
> On Thu, Jan 15, 2009 at 6:06 AM, Pádraig Brady <P@draigbrady.com> 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 <rob.krakora@messagenetsystems.com>
>>>
>>> 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