[PATCH 1/3] avcodec/ass: Faster ff_ass_add_rect()
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ libavcodec/ass.h | 7 +++++++ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/libavcodec/ass.c b/libavcodec/ass.c index 725e4d42ba1..06714678722 100644 --- a/libavcodec/ass.c +++ b/libavcodec/ass.c @@ -114,17 +114,31 @@ char *ff_ass_get_dialog(int readorder, int layer, const char *style, speaker ? speaker : "", text); } -int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog, int readorder, int layer, const char *style, - const char *speaker) + const char *speaker, unsigned *nb_rect_allocated) { - AVSubtitleRect **rects, *rect; + AVSubtitleRect **rects = sub->rects, *rect; char *ass_str; + uint64_t new_nb = 0; - rects = av_realloc_array(sub->rects, sub->num_rects+1, sizeof(*sub->rects)); - if (!rects) + if (sub->num_rects >= UINT_MAX) return AVERROR(ENOMEM); - sub->rects = rects; + + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX); + } else if (!nb_rect_allocated) + new_nb = sub->num_rects + 1LL; + + if (new_nb) { + rects = av_realloc_array(rects, new_nb, sizeof(*sub->rects)); + if (!rects) + return AVERROR(ENOMEM); + if (nb_rect_allocated) + *nb_rect_allocated = new_nb; + sub->rects = rects; + } + rect = av_mallocz(sizeof(*rect)); if (!rect) return AVERROR(ENOMEM); @@ -137,6 +151,13 @@ int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, return 0; } +int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, + int readorder, int layer, const char *style, + const char *speaker) +{ + return ff_ass_add_rect2(sub, dialog, readorder, layer, style, speaker, NULL); +} + void ff_ass_decoder_flush(AVCodecContext *avctx) { FFASSDecoderContext *s = avctx->priv_data; diff --git a/libavcodec/ass.h b/libavcodec/ass.h index 2c260e4e785..4dffe923d9f 100644 --- a/libavcodec/ass.h +++ b/libavcodec/ass.h @@ -118,6 +118,13 @@ int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, int readorder, int layer, const char *style, const char *speaker); +/** + * Add an ASS dialog to a subtitle. + */ +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog, + int readorder, int layer, const char *style, + const char *speaker, unsigned *nb_rect_allocated); + /** * Helper to flush a text subtitles decoder making use of the * FFASSDecoderContext. -- 2.17.1
Fixes: Timeout Fixes: 42258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CCAPTION_fuzzer-5540144118104064 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ccaption_dec.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c index 27c61527f6e..15be18eb164 100644 --- a/libavcodec/ccaption_dec.c +++ b/libavcodec/ccaption_dec.c @@ -850,6 +850,7 @@ static int decode(AVCodecContext *avctx, void *data, int *got_sub, AVPacket *avp int len = avpkt->size; int ret = 0; int i; + unsigned nb_rect_allocated = 0; for (i = 0; i < len; i += 3) { uint8_t hi, cc_type = bptr[i] & 1; @@ -886,7 +887,7 @@ static int decode(AVCodecContext *avctx, void *data, int *got_sub, AVPacket *avp AV_TIME_BASE_Q, ms_tb); else sub->end_display_time = -1; - ret = ff_ass_add_rect(sub, ctx->buffer[bidx].str, ctx->readorder++, 0, NULL, NULL); + ret = ff_ass_add_rect2(sub, ctx->buffer[bidx].str, ctx->readorder++, 0, NULL, NULL, &nb_rect_allocated); if (ret < 0) return ret; ctx->last_real_time = sub->pts; @@ -896,7 +897,7 @@ static int decode(AVCodecContext *avctx, void *data, int *got_sub, AVPacket *avp if (!bptr && !ctx->real_time && ctx->buffer[!ctx->buffer_index].str[0]) { bidx = !ctx->buffer_index; - ret = ff_ass_add_rect(sub, ctx->buffer[bidx].str, ctx->readorder++, 0, NULL, NULL); + ret = ff_ass_add_rect2(sub, ctx->buffer[bidx].str, ctx->readorder++, 0, NULL, NULL, &nb_rect_allocated); if (ret < 0) return ret; sub->pts = ctx->buffer_time[1]; @@ -914,7 +915,7 @@ static int decode(AVCodecContext *avctx, void *data, int *got_sub, AVPacket *avp capture_screen(ctx); ctx->buffer_changed = 0; - ret = ff_ass_add_rect(sub, ctx->buffer[bidx].str, ctx->readorder++, 0, NULL, NULL); + ret = ff_ass_add_rect2(sub, ctx->buffer[bidx].str, ctx->readorder++, 0, NULL, NULL, &nb_rect_allocated); if (ret < 0) return ret; sub->end_display_time = -1; -- 2.17.1
On Mon, Dec 20, 2021 at 12:56:59AM +0100, Michael Niedermayer wrote:
Fixes: Timeout Fixes: 42258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CCAPTION_fuzzer-5540144118104064
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ccaption_dec.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
will apply [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle
Fixes: Timeout (same growing chunk is decoded to failure repeatedly) Fixes: 42582/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VQA_fuzzer-6531195591065600 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vqavideo.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libavcodec/vqavideo.c b/libavcodec/vqavideo.c index 7c1d42bcacc..1d97855e606 100644 --- a/libavcodec/vqavideo.c +++ b/libavcodec/vqavideo.c @@ -608,13 +608,14 @@ static int vqa_decode_frame_pal8(VqaContext *s, AVFrame *frame) if (s->partial_countdown <= 0) { bytestream2_init(&s->gb, s->next_codebook_buffer, s->next_codebook_buffer_index); /* decompress codebook */ - if ((res = decode_format80(s, s->next_codebook_buffer_index, - s->codebook, s->codebook_size, 0)) < 0) - return res; + res = decode_format80(s, s->next_codebook_buffer_index, + s->codebook, s->codebook_size, 0); /* reset accounting */ s->next_codebook_buffer_index = 0; s->partial_countdown = s->partial_count; + if (res < 0) + return res; } } -- 2.17.1
On Mon, Dec 20, 2021 at 12:57:00AM +0100, Michael Niedermayer wrote:
Fixes: Timeout (same growing chunk is decoded to failure repeatedly) Fixes: 42582/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VQA_fuzzer-6531195591065600
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vqavideo.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
will apply [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On 12/19/2021 8:56 PM, Michael Niedermayer wrote:
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ libavcodec/ass.h | 7 +++++++ 2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/libavcodec/ass.c b/libavcodec/ass.c index 725e4d42ba1..06714678722 100644 --- a/libavcodec/ass.c +++ b/libavcodec/ass.c @@ -114,17 +114,31 @@ char *ff_ass_get_dialog(int readorder, int layer, const char *style, speaker ? speaker : "", text); }
-int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog, int readorder, int layer, const char *style, - const char *speaker) + const char *speaker, unsigned *nb_rect_allocated) { - AVSubtitleRect **rects, *rect; + AVSubtitleRect **rects = sub->rects, *rect; char *ass_str; + uint64_t new_nb = 0;
- rects = av_realloc_array(sub->rects, sub->num_rects+1, sizeof(*sub->rects)); - if (!rects) + if (sub->num_rects >= UINT_MAX) return AVERROR(ENOMEM); - sub->rects = rects; + + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX);
Isn't this what av_fast_realloc() is for?
+ } else if (!nb_rect_allocated) + new_nb = sub->num_rects + 1LL; + + if (new_nb) { + rects = av_realloc_array(rects, new_nb, sizeof(*sub->rects)); + if (!rects) + return AVERROR(ENOMEM); + if (nb_rect_allocated) + *nb_rect_allocated = new_nb; + sub->rects = rects; + } + rect = av_mallocz(sizeof(*rect)); if (!rect) return AVERROR(ENOMEM); @@ -137,6 +151,13 @@ int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, return 0; }
+int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, + int readorder, int layer, const char *style, + const char *speaker) +{ + return ff_ass_add_rect2(sub, dialog, readorder, layer, style, speaker, NULL); +} + void ff_ass_decoder_flush(AVCodecContext *avctx) { FFASSDecoderContext *s = avctx->priv_data; diff --git a/libavcodec/ass.h b/libavcodec/ass.h index 2c260e4e785..4dffe923d9f 100644 --- a/libavcodec/ass.h +++ b/libavcodec/ass.h @@ -118,6 +118,13 @@ int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, int readorder, int layer, const char *style, const char *speaker);
+/** + * Add an ASS dialog to a subtitle. + */ +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog, + int readorder, int layer, const char *style, + const char *speaker, unsigned *nb_rect_allocated); + /** * Helper to flush a text subtitles decoder making use of the * FFASSDecoderContext.
On Sun, Dec 19, 2021 at 09:00:27PM -0300, James Almer wrote:
On 12/19/2021 8:56 PM, Michael Niedermayer wrote:
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ libavcodec/ass.h | 7 +++++++ 2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/libavcodec/ass.c b/libavcodec/ass.c index 725e4d42ba1..06714678722 100644 --- a/libavcodec/ass.c +++ b/libavcodec/ass.c @@ -114,17 +114,31 @@ char *ff_ass_get_dialog(int readorder, int layer, const char *style, speaker ? speaker : "", text); } -int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog, int readorder, int layer, const char *style, - const char *speaker) + const char *speaker, unsigned *nb_rect_allocated) { - AVSubtitleRect **rects, *rect; + AVSubtitleRect **rects = sub->rects, *rect; char *ass_str; + uint64_t new_nb = 0; - rects = av_realloc_array(sub->rects, sub->num_rects+1, sizeof(*sub->rects)); - if (!rects) + if (sub->num_rects >= UINT_MAX) return AVERROR(ENOMEM); - sub->rects = rects; + + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX);
Isn't this what av_fast_realloc() is for?
No, its what av_fast_realloc_array() would be for but that is not in git yet I think we should wait for that to become available before changing the used function (1 change less / less work) thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle
Michael Niedermayer:
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ libavcodec/ass.h | 7 +++++++ 2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/libavcodec/ass.c b/libavcodec/ass.c index 725e4d42ba1..06714678722 100644 --- a/libavcodec/ass.c +++ b/libavcodec/ass.c @@ -114,17 +114,31 @@ char *ff_ass_get_dialog(int readorder, int layer, const char *style, speaker ? speaker : "", text); }
-int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog, int readorder, int layer, const char *style, - const char *speaker) + const char *speaker, unsigned *nb_rect_allocated) { - AVSubtitleRect **rects, *rect; + AVSubtitleRect **rects = sub->rects, *rect; char *ass_str; + uint64_t new_nb = 0;
- rects = av_realloc_array(sub->rects, sub->num_rects+1, sizeof(*sub->rects)); - if (!rects) + if (sub->num_rects >= UINT_MAX) return AVERROR(ENOMEM); - sub->rects = rects; + + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX);
This presumes that unsigned is not 64bits itself; I have no problem with this, so LGTM from me. Others may disagree.
+ } else if (!nb_rect_allocated) + new_nb = sub->num_rects + 1LL;
+1 is enough (it has been checked that sub->num_rects is < UINT_MAX).
+ + if (new_nb) { + rects = av_realloc_array(rects, new_nb, sizeof(*sub->rects)); + if (!rects) + return AVERROR(ENOMEM); + if (nb_rect_allocated) + *nb_rect_allocated = new_nb; + sub->rects = rects; + } + rect = av_mallocz(sizeof(*rect)); if (!rect) return AVERROR(ENOMEM); @@ -137,6 +151,13 @@ int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, return 0; }
+int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, + int readorder, int layer, const char *style, + const char *speaker) +{ + return ff_ass_add_rect2(sub, dialog, readorder, layer, style, speaker, NULL); +} + void ff_ass_decoder_flush(AVCodecContext *avctx) { FFASSDecoderContext *s = avctx->priv_data; diff --git a/libavcodec/ass.h b/libavcodec/ass.h index 2c260e4e785..4dffe923d9f 100644 --- a/libavcodec/ass.h +++ b/libavcodec/ass.h @@ -118,6 +118,13 @@ int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, int readorder, int layer, const char *style, const char *speaker);
+/** + * Add an ASS dialog to a subtitle. + */ +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog, + int readorder, int layer, const char *style, + const char *speaker, unsigned *nb_rect_allocated); + /** * Helper to flush a text subtitles decoder making use of the * FFASSDecoderContext.
On Mon, Dec 20, 2021 at 01:39:47AM +0100, Andreas Rheinhardt wrote:
Michael Niedermayer:
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ libavcodec/ass.h | 7 +++++++ 2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/libavcodec/ass.c b/libavcodec/ass.c index 725e4d42ba1..06714678722 100644 --- a/libavcodec/ass.c +++ b/libavcodec/ass.c @@ -114,17 +114,31 @@ char *ff_ass_get_dialog(int readorder, int layer, const char *style, speaker ? speaker : "", text); }
-int ff_ass_add_rect(AVSubtitle *sub, const char *dialog, +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog, int readorder, int layer, const char *style, - const char *speaker) + const char *speaker, unsigned *nb_rect_allocated) { - AVSubtitleRect **rects, *rect; + AVSubtitleRect **rects = sub->rects, *rect; char *ass_str; + uint64_t new_nb = 0;
- rects = av_realloc_array(sub->rects, sub->num_rects+1, sizeof(*sub->rects)); - if (!rects) + if (sub->num_rects >= UINT_MAX) return AVERROR(ENOMEM); - sub->rects = rects; + + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX);
This presumes that unsigned is not 64bits itself; I have no problem with this, so LGTM from me. Others may disagree.
if unsigned is 64bit how would you exploit this ? for this to overflow you would first need close to 2^64 successfully allocated rectangles. Thats both alot of space and time. I mean if every human on earth had a 32gb stick then all these together in a single computer would not be enough to allow this to succeed and that has to happen before the function is changed to the correct size_t type as my previous patch did. will post a new patch thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong.
participants (3)
-
Andreas Rheinhardt -
James Almer -
Michael Niedermayer