[FFmpeg-devel] [PATCH 1/2] avcodec/htmlsubtitles: Protect very slow redundant sscanf() calls by optimized use of strchr()

[wm4]

On Fri, 9 Jun 2017 19:39:36 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Jun 09, 2017 at 04:32:41PM +0200, wm4 wrote:
> > On Thu,  8 Jun 2017 23:53:55 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > Fixes Timeout
> > > Fixes: 2127/clusterfuzz-testcase-minimized-6595787859427328
> > > 
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavcodec/htmlsubtitles.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavcodec/htmlsubtitles.c b/libavcodec/htmlsubtitles.c
> > > index 16295daa0c..ba4f269b3f 100644
> > > --- a/libavcodec/htmlsubtitles.c
> > > +++ b/libavcodec/htmlsubtitles.c
> > > @@ -56,6 +56,7 @@ int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in)
> > >      char *param, buffer[128], tmp[128];
> > >      int len, tag_close, sptr = 1, line_start = 1, an = 0, end = 0;
> > >      SrtStack stack[16];
> > > +    const char *next_closep = NULL;
> > >  
> > >      stack[0].tag[0] = 0;
> > >      strcpy(stack[0].param[PARAM_SIZE],  "{\\fs}");
> > > @@ -83,8 +84,15 @@ int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in)
> > >                          and all microdvd like styles such as {Y:xxx} */
> > >              len = 0;
> > >              an += sscanf(in, "{\\an%*1u}%n", &len) >= 0 && len > 0;
> > > -            if ((an != 1 && (len = 0, sscanf(in, "{\\%*[^}]}%n", &len) >= 0 && len > 0)) ||
> > > -                (len = 0, sscanf(in, "{%*1[CcFfoPSsYy]:%*[^}]}%n", &len) >= 0 && len > 0)) {
> > > +
> > > +            if(!next_closep || next_closep <= in) {
> > > +                next_closep = strchr(in+1, '}');
> > > +                if (!next_closep)
> > > +                    next_closep = in + strlen(in);
> > > +            }
> > > +
> > > +            if (*next_closep == '}' && (an != 1 && (len = 0, sscanf(in, "{\\%*[^}]}%n", &len) >= 0 && len > 0)) ||
> > > +                *next_closep == '}' && (len = 0, sscanf(in, "{%*1[CcFfoPSsYy]:%*[^}]}%n", &len) >= 0 && len > 0)) {
> > >                  in += len - 1;
> > >              } else
> > >                  av_bprint_chars(dst, *in, 1);  
> > 
> > I'm not convinced that bad performance with an obscure fuzzed sample is
> > worth the complexity increase here.  
> 
> If we want to make ff_htmlmarkup_to_ass() not go into near infinite
> loops with crafted data then this or a equivalent change is likely
> needed.

Maybe if someone starts abusing this case.

> 
> > 
> > I'd rather ask, why the heck is it using sscanf in the first place?  
> 
> I suspect the use of scanf is fewer lines of code than the alternative
> but iam not the author of this, i dont know for sure why it was written
> as it was.

I'd probably prefer explicit code here (as long as it's not typical
"tricky C string processing" style), but it's hard to tell.