[FFmpeg-devel] [PATCH] libavfilter/vf_drawtext: Avoid undefined behavior from GET_UTF8

[Michael Niedermayer]

On Sat, Jul 27, 2019 at 07:58:48AM -0700, Aaron Boushley wrote:
> The vf_drawtext filter uses the GET_UTF8 macro in multiple locations.
> Each of these use `continue;` as the error handler. However the
> documentation for the GET_UTF8 macro states "ERROR should not contain
> a loop control statement which could interact with the internal while
> loop, and should force an exit from the macro code (e.g. through a
> goto or a return) in order to prevent undefined results."
> 
> This patch adjusts vf_drawtext to use goto error handlers similar to
> other locations in ffmpeg.
> 
> Aaron
> 
> PS Sorry for having to send again, sent from the wrong address last
> time, so patchwork didn't pick it up.
> 

>  vf_drawtext.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 04550ed40f98ec78c2719de31da889e92ff42f45  libavfilter-drawtext-avoid-undefined-behavior.patch
> From efdc96ace59d676e76434499a399d1d7df7fa093 Mon Sep 17 00:00:00 2001
> From: Aaron Boushley <boushley at gmail.com>
> Date: Fri, 26 Jul 2019 15:49:36 -0700
> Subject: [PATCH] libavfilter/drawtext: avoid undefined behavior with GET_UTF8
> 
> Currently the GET_UTF8 usage in drawtext use a continue to skip invalid
> characters in a string. The macro definition states that using a loop
> control statement results in undefined behavior since the macro itself
> uses a loop.
> 
> This switches drawtext to use a goto statement similar to other usages
> of GET_UTF8 in other parts of ffmpeg.
> 
> Signed-off-by: Aaron Boushley <boushley at gmail.com>
> ---
>  libavfilter/vf_drawtext.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c
> index 8f4badbdb5..fd2ba84d12 100644
> --- a/libavfilter/vf_drawtext.c
> +++ b/libavfilter/vf_drawtext.c
> @@ -1223,7 +1223,7 @@ static int draw_glyphs(DrawTextContext *s, AVFrame *frame,
>      for (i = 0, p = text; *p; i++) {
>          FT_Bitmap bitmap;
>          Glyph dummy = { 0 };
> -        GET_UTF8(code, *p++, continue;);
> +        GET_UTF8(code, *p++, goto invalid_drawing;);
>  
>          /* skip new line chars, just go to new line */
>          if (code == '\n' || code == '\r' || code == '\t')
> @@ -1248,6 +1248,9 @@ static int draw_glyphs(DrawTextContext *s, AVFrame *frame,
>                        bitmap.width, bitmap.rows,
>                        bitmap.pixel_mode == FT_PIXEL_MODE_MONO ? 0 : 3,
>                        0, x1, y1);
> +        continue;
> +invalid_drawing:
> +        av_log(s, AV_LOG_DEBUG, "Invalid UTF8 character while drawing glyphs\n");
>      }

Why does this add av_log() calls ?
This seems unrelated to the bug fix. Also why are they debug level ?
if a user draws a string she provided that could be argued to be an error
the user wants to know about. But maybe iam missing something
Also they probably should provide more information about where the
invalid character is. It may be hard to find in a long text for some users

Thanks

[...]

-- 
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: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190909/945cfde7/attachment.sig>