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

[wm4]

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.

I'd rather ask, why the heck is it using sscanf in the first place?
The existing code is certainly unreadable already. (Could you tell what
it does without staring at the scanf manpage for a while? And then
guarantee correctness/performance/security?)