| Message ID | d95c1bfa-10e8-95d1-4cfc-92ae4ed2293a@gmail.com |
|---|---|
| State | New |
| Headers | show |
Can I get a review on this. Anywhere we have code calling out and performing undefined behavior we should clean that up. Aaron On Sat, Jul 27, 2019 at 7:58 AM Aaron Boushley <boushley@gmail.com> 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. >
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@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@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 [...]
I'm open to suggestions of alternatives. I want sure if I could do a goto if there was no code at that location so I chose to add a log statement figuring that would be helpful if you had problems. I chose debug because previously the character was completely ignored, so didn't want to go from no notification to an error log. I could also just put a continue call there, or do more research to figure out if I can go to without having any content on the line. Suggestions for what I should do differently with that context? Aaron On Mon, Sep 9, 2019, 9:13 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > 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@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@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 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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"); } return 0; @@ -1361,7 +1364,7 @@ static int draw_text(AVFilterContext *ctx, AVFrame *frame, /* load and cache glyphs */ for (i = 0, p = text; *p; i++) { - GET_UTF8(code, *p++, continue;); + GET_UTF8(code, *p++, goto invalid_caching;); /* get glyph */ dummy.code = code; @@ -1377,6 +1380,10 @@ static int draw_text(AVFilterContext *ctx, AVFrame *frame, y_max = FFMAX(glyph->bbox.yMax, y_max); x_min = FFMIN(glyph->bbox.xMin, x_min); x_max = FFMAX(glyph->bbox.xMax, x_max); + + continue; +invalid_caching: + av_log(ctx, AV_LOG_DEBUG, "Invalid UTF8 character while caching glyphs\n"); } s->max_glyph_h = y_max - y_min; s->max_glyph_w = x_max - x_min; @@ -1384,7 +1391,7 @@ static int draw_text(AVFilterContext *ctx, AVFrame *frame, /* compute and save position for each glyph */ glyph = NULL; for (i = 0, p = text; *p; i++) { - GET_UTF8(code, *p++, continue;); + GET_UTF8(code, *p++, goto invalid_positioning;); /* skip the \n in the sequence \r\n */ if (prev_code == '\r' && code == '\n') @@ -1417,6 +1424,10 @@ static int draw_text(AVFilterContext *ctx, AVFrame *frame, s->positions[i].y = y - glyph->bitmap_top + y_max; if (code == '\t') x = (x / s->tabsize + 1)*s->tabsize; else x += glyph->advance; + + continue; +invalid_positioning: + av_log(ctx, AV_LOG_DEBUG, "Invalid UTF8 character while positioning glyphs\n"); } max_text_line_w = FFMAX(x, max_text_line_w);