[FFmpeg-devel] [PATCH] Fix segv when decoding gray8

[Baptiste Coudurier]

On 06/03/2010 02:43 PM, Michael Niedermayer wrote:
> On Thu, Jun 03, 2010 at 02:34:47PM -0700, Baptiste Coudurier wrote:
>> On 06/03/2010 02:22 PM, Michael Niedermayer wrote:
>>> On Thu, Jun 03, 2010 at 01:57:22PM -0700, Baptiste Coudurier wrote:
>>>> On 06/03/2010 01:48 PM, Michael Niedermayer wrote:
>>>>> On Thu, Jun 03, 2010 at 12:11:41PM -0700, Baptiste Coudurier wrote:
>>>>>> On 06/03/2010 03:36 AM, Michael Niedermayer wrote:
>>>>>>> On Wed, Jun 02, 2010 at 06:39:26PM -0700, Baptiste Coudurier wrote:
>>>>>>>> Hi guys,
>>>>>>>>
>>>>>>>> $subject.
>>>>>>>>
>>>>>>>> frame->data[1] is set by avpicture_fill which is called with buf
>>>>>>>> passed
>>>>>>>> from the demuxer.
>>>>>>>> However, for gray8 the data stored is only width*height, so is too
>>>>>>>> small
>>>>>>>> for the palette, and will segv, memcpying.
>>>>>>>>
>>>>>>>> I'm not sure what is the right fix here, buf is supposed to be const.
>>>>>>>
>>>>>>> as this case can only arrise from use of deprecated (and buggy)
>>>>>>> palette
>>>>>>> passing API, the correct (long term goal) is to change all codecs so
>>>>>>> they
>>>>>>> finally stop using this highly unpredictable race condition ridden
>>>>>>> api.
>>>>>>> until then, your patch may be ok
>>>>>>
>>>>>> I don't think this is related to the API.
>>>>>>
>>>>>> You cannot generate the palette for gray8 (according to pixfmt.h)
>>>>>> _anyway_
>>>>>> since the data buffer is not big enough, since it comes from the
>>>>>> demuxer
>>>>>> itself.
>>>>>> So either, the picture needs to be copied and the palette generated, or
>>>>>> we
>>>>>> skip the palette generation.
>>>>>
>>>>> maybe iam missing something but why cant  data[1] that contains the
>>>>> palette
>>>>> point to some pal[256] from the context while data[0] would point to the
>>>>> buffer that originates from the demuxer?
>>>>
>>>> Yes, that should be ok assuming there is instructions about only freeing
>>>> data[0], this is what avpicture_free does, but it seems not documented
>>>> anywhere.
>>>
>>> maybe iam silly but i dont think the frames returned by the raw decoder
>>> are
>>> freed by avpicture_free() but rather by av_free_packet() or in
>>> raw_close_decoder()
>>
>> How is the user supposed to free the AVFrame given by the decoder ?
>
> with default get/release_buffer(), the user does not, the decoder does.
> with overridden get/release_buffer() the user decides into what buffer
> the decoder decodes and the decoder tells the user once it does not need
> the data anymore

Ok, this should be documented.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org