[FFmpeg-devel] [PATCH] lavu/common: make GET_UTF8 less broken

Stefano Sabatini stefasab at gmail.com
Thu Oct 3 00:26:13 CEST 2013


On date Wednesday 2013-10-02 20:37:20 +0200, Reimar Döffinger encoded:
> On Wed, Oct 02, 2013 at 08:22:05PM +0200, Stefano Sabatini wrote:
> > On date Wednesday 2013-10-02 20:17:30 +0200, Stefano Sabatini encoded:
> > > Avoid to process data and push the pointer forward in case as soon as an
> > > error is encountered.
> > > 
> > > For example if the error is an assignment, this will allow to exit the
> > > while block as soon as possible.
> > > ---
> > >  libavutil/common.h | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/libavutil/common.h b/libavutil/common.h
> > > index 9f0f998..58f7e42 100644
> > > --- a/libavutil/common.h
> > > +++ b/libavutil/common.h
> > > @@ -300,14 +300,20 @@ static av_always_inline av_const int av_popcount64_c(uint64_t x)
> > >      val= GET_BYTE;\
> > >      {\
> > >          uint32_t top = (val & 128) >> 1;\
> > > -        if ((val & 0xc0) == 0x80 || val >= 0xFE)\
> > > +        int err = 0;\
> > > +        if ((val & 0xc0) == 0x80 || val >= 0xFE) {\
> > > +            err = 1;\
> > >              ERROR\
> > > -        while (val & top) {\
> > > +        }\
> > > +        while (!err && (val & top)) {\
> > >              int tmp= GET_BYTE - 128;\
> > > -            if(tmp>>6)\
> > > +            if (tmp>>6) {\
> > > +                err = 1;\
> > >                  ERROR\
> > > -            val= (val<<6) + tmp;\
> > > -            top <<= 5;\
> > > +            } else {\
> > > +                val x = (val<<6) + tmp;\
> > > +                top <<= 5;\
> > > +            }\
> > >          }\
> > >          val &= (top << 1) - 1;\
> > >      }
> > 
> > Other problems: in the while loop, the characters should probably not
> > consumed, and the pointer restored to the second character.
> > 
> > In case ERROR is a struck control operation (e.g. "continue;" or
> > "break;") it will affect the while loop context, and not the external
> > user loop as intended.
> > 
> > In conclusion we should probably replace this with an inline function
> > or something faintly robust.
> 

> I think this is supposed to be a high-performance primitive,
> and ERROR is not supposed to be anything other than a return
> or goto.
> "Bloating" it even more IMHO defeats the purpose.
> I'd suggest providing a (possibly inline) function that
> is less tricky to use, and warning that this function is
> not intended to easily allow complex error handling.

Grepping for GET_UTF8:
./libavfilter/vf_drawtext.c:820:        GET_UTF8(code, *p++, continue;);
./libavfilter/vf_drawtext.c:910:        GET_UTF8(code, *p++, continue;);
./libavfilter/vf_drawtext.c:930:        GET_UTF8(code, *p++, continue;);
./libavcodec/flac.c:46:    GET_UTF8(val, get_bits(gb, 8), return -1;)
./libavcodec/utils.c:2310:        GET_UTF8(codepoint, *(byte++), return 0;);
./libavutil/common.h:299:#define GET_UTF8(val, GET_BYTE, ERROR)\
./libavformat/aviobuf.c:327:        GET_UTF8(ch, *q++, break;)
./libavformat/movenc.c:2141:        GET_UTF8(val, *b++, return -1;)
./libavformat/movenc.c:2151:        GET_UTF8(val, *b++, return -1;)

while I should probably get rid of GET_UTF8 in vf_drawtext.c (which is
broken in several ways), the use in aviobuf.c seems potentially unsafe
and should be fixed.

Also I'm not sure about utf8_check(), its functionality should be
probably merged in the new routine.
-- 
FFmpeg = Fancy Fancy Merciful Ponderous Elastic Guru


More information about the ffmpeg-devel mailing list