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

[Michael Niedermayer]

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 could check at least for
codec_id being NONE or the correct one and type being unknown or the
correct one
patch welcome

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20090923/037153bd/attachment-0001.pgp>