[FFmpeg-devel] Security issues?

[Reimar Döffinger]

On Wed, Sep 23, 2009 at 05:28:25PM +0200, Michael Niedermayer wrote:
> On Wed, Sep 23, 2009 at 05:02:49PM +0200, Reimar D?ffinger wrote:
> > On Wed, Sep 23, 2009 at 03:22:02PM +0200, Michael Niedermayer wrote:
> > > On Wed, Sep 23, 2009 at 03:15:06PM +0200, Reimar D?ffinger wrote:
> > > > On Wed, Sep 23, 2009 at 02:57:31PM +0200, Michael Niedermayer wrote:
> > > > > > Index: libavcodec/vorbis_dec.c
> > > > > > ===================================================================
> > > > > > --- libavcodec/vorbis_dec.c     (revision 19987)
> > > > > > +++ libavcodec/vorbis_dec.c     (working copy)
> > > > > > @@ -162,6 +162,13 @@
> > > > > >  #define BARK(x) \
> > > > > >      (13.1f*atan(0.00074f*(x))+2.24f*atan(1.85e-8f*(x)*(x))+1e-4f*(x))
> > > > > >  
> > > > > > +
> > > > > > +#define VALIDATE_INDEX(ctx, idx, limit, onerror) \
> > > > > 
> > > > > > +    if (idx >= limit) {\
> > > > > 
> > > > > the check should be an argument to the macro
> > > > > 
> > > > > 
> > > > > > +        av_log(ctx, AV_LOG_ERROR, "Index value %d out of range (0 - %d) for "#idx "\n", idx, limit);\
> > > > 
> > > > Uh, and what should be the message then? If you change the check, this
> > > > message no longer makes sense, 
> > > 
> > > the message could be an argument too
> > 
> > The message and the arguments then. And IMO then you don't really save
> > much.
> > For what guess, there are maybe 10 places where this macro can be
> > used unchanged and a few where you'd have to add a if (... != -1),
> > and I just think a macro for this special case alone would be justified
> > and making it more generic just re-adds some of the bloat it is supposed
> > to avoid...
> 
> blah ...
> 
> #define MACRO(dst, cond, bits, err)\
> unsigned int tmp= get_bits(bits);\
> if(!(cond)){\
>     av_log( #dst " fails the condition " #cond "\n");\
>     return err
> }

Which prints almost no useless information. Actually it no runtime
information _at all_.
Anyway, I already sent a patch, comment there.