[FFmpeg-cvslog] r19973 - trunk/libavcodec/utils.c

[Baptiste Coudurier]

On 09/23/2009 11:56 AM, Michael Niedermayer wrote:
> On Wed, Sep 23, 2009 at 08:35:19PM +0200, Reimar D?ffinger wrote:
>> On Wed, Sep 23, 2009 at 04:53:11PM +0200, Michael Niedermayer wrote:
>>> On Wed, Sep 23, 2009 at 04:25:22AM +0300, Uoti Urpala wrote:
>>>> On Wed, 2009-09-23 at 00:44 +0200, michael wrote:
>>>>> Log:
>>>>> Check codec_id and codec_type in avcodec_open(), based on 43_codec_type_mismatch.patch from chrome
>>>>> This is said to be able to lead to a stack based buffer overflow.
>>>>>
>>>>> Modified:
>>>>>     trunk/libavcodec/utils.c
>>>>>
>>>>> Modified: trunk/libavcodec/utils.c
>>>>> ==============================================================================
>>>>> --- trunk/libavcodec/utils.c	Tue Sep 22 22:38:03 2009	(r19972)
>>>>> +++ trunk/libavcodec/utils.c	Wed Sep 23 00:44:56 2009	(r19973)
>>>>> @@ -481,7 +481,10 @@ int attribute_align_arg avcodec_open(AVC
>>>>>       }
>>>>>
>>>>>       avctx->codec = codec;
>>>>> -    avctx->codec_id = codec->id;
>>>>> +    if(avctx->codec_id != codec->id || avctx->codec_type != codec->type){
>>>>> +        av_log(avctx, AV_LOG_ERROR, "codec type or id mismatches\n");
>>>>> +        goto end;
>>>>> +    }
>>>>
>>>> What's the point of this? Is the application supposed to set those
>>>> before calling avcodec_open()? If so then why couldn't FFmpeg set them
>>>> just as well instead of checking they're already set? Or what kind of
>>>> usage is assumed where they could already be set to different values and
>>>> checking it could be meaningful? Is this about FFmpeg itself using
>>>> avcodec_open() internally in an unsafe way?
>>>>
>>>> At least MPlayer does not set avctx->codec_id or avctx->codec_type
>>>> before calling avcodec_open() and so wouldn't work with this change. Of
>>>> course setting them would be trivial, but does requiring that really
>>>> make sense for the API?
>>>
>>> A patch that sets codec_id to codec->codec_id if they are both at
>>> CODEC_ID_NONE/CODEC_TYPE_"correct" is welcome
>>> codec_type has to be correct because avcodec_alloc_context2() sets it
>>
>> Uh... avcodec_alloc_context is the current API, avcodec_alloc_context2
>> isn't even public yet.
>
> ooops
>
> i guess in that case besides that a avcodec_alloc_context3 with a
> codec_id as argument would be a good idea ...

We were discussing this when we added avcodec_alloc_context2, this is 
the door for per codec defaults too :>

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