| Message ID | GV1P250MB07378E22286068ABDF5CE5A88F469@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 62af385b917f2474498abeabd6057e6c60e2b9a9 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/dump: Avoid unnecessary implicit calculation of strlen | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 9/14/22, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > av_strlcpy() returns the length of the src string to enable > the caller to check for truncation. It is currently used in > the following way in dump_metadata(): Every metadata value > is searched for \b, \n, \v, \f, \r and then the data up to > the first of these characters found is copied to a small > temporary buffer via av_strlcpy() (but of course not more > than fits into said buffer) and then printed; all characters up > to the character found earlier are then treated as consumed. > > But this is bad performance-wise if the while string is big > and contains many of these characters, because av_strlcpy() > will unnecessarily calculate the length of the whole remaining string. > (dump_metadata() actually ignored the return value of av_strlcpy().) > > Fix this by just not copying the data in a temporary buffer at all. > Instead just use the %.*s to bound the number of characters output. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavformat/dump.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/libavformat/dump.c b/libavformat/dump.c > index e3f0056c20..cafcef36c6 100644 > --- a/libavformat/dump.c > +++ b/libavformat/dump.c > @@ -145,10 +145,8 @@ static void dump_metadata(void *ctx, const AVDictionary > *m, const char *indent) > av_log(ctx, AV_LOG_INFO, > "%s %-16s: ", indent, tag->key); > while (*p) { > - char tmp[256]; > size_t len = strcspn(p, "\x8\xa\xb\xc\xd"); > - av_strlcpy(tmp, p, FFMIN(sizeof(tmp), len+1)); > - av_log(ctx, AV_LOG_INFO, "%s", tmp); > + av_log(ctx, AV_LOG_INFO, "%.*s", (int)(FFMIN(255, > len)), p); > p += len; > if (*p == 0xd) av_log(ctx, AV_LOG_INFO, " "); > if (*p == 0xa) av_log(ctx, AV_LOG_INFO, "\n%s %-16s: > ", indent, ""); > -- > 2.34.1 > > _______________________________________________ > 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". > LGTM
diff --git a/libavformat/dump.c b/libavformat/dump.c index e3f0056c20..cafcef36c6 100644 --- a/libavformat/dump.c +++ b/libavformat/dump.c @@ -145,10 +145,8 @@ static void dump_metadata(void *ctx, const AVDictionary *m, const char *indent) av_log(ctx, AV_LOG_INFO, "%s %-16s: ", indent, tag->key); while (*p) { - char tmp[256]; size_t len = strcspn(p, "\x8\xa\xb\xc\xd"); - av_strlcpy(tmp, p, FFMIN(sizeof(tmp), len+1)); - av_log(ctx, AV_LOG_INFO, "%s", tmp); + av_log(ctx, AV_LOG_INFO, "%.*s", (int)(FFMIN(255, len)), p); p += len; if (*p == 0xd) av_log(ctx, AV_LOG_INFO, " "); if (*p == 0xa) av_log(ctx, AV_LOG_INFO, "\n%s %-16s: ", indent, "");
av_strlcpy() returns the length of the src string to enable the caller to check for truncation. It is currently used in the following way in dump_metadata(): Every metadata value is searched for \b, \n, \v, \f, \r and then the data up to the first of these characters found is copied to a small temporary buffer via av_strlcpy() (but of course not more than fits into said buffer) and then printed; all characters up to the character found earlier are then treated as consumed. But this is bad performance-wise if the while string is big and contains many of these characters, because av_strlcpy() will unnecessarily calculate the length of the whole remaining string. (dump_metadata() actually ignored the return value of av_strlcpy().) Fix this by just not copying the data in a temporary buffer at all. Instead just use the %.*s to bound the number of characters output. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavformat/dump.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)