* [FFmpeg-devel] [h264] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE @ 2025-08-05 21:52 Dale Curtis 2025-08-06 22:05 ` Michael Niedermayer 0 siblings, 1 reply; 3+ messages in thread From: Dale Curtis @ 2025-08-05 21:52 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1: Type: text/plain, Size: 248 bytes --] Don't silently skip errors when AV_EF_EXPLODE is specified. This can lead to out-of-bound reads with ff_put_h264_chroma_mc4_ssse3() when small padding is used with the checked bitstream reader. Signed-off-by: Dale Curtis <dalecurtis@chromium.org> [-- Attachment #2: h264_stricter_v1.patch --] [-- Type: application/octet-stream, Size: 2009 bytes --] [-- Attachment #3: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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". ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [FFmpeg-devel] [h264] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE 2025-08-05 21:52 [FFmpeg-devel] [h264] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE Dale Curtis @ 2025-08-06 22:05 ` Michael Niedermayer 2025-08-07 21:20 ` Dale Curtis 0 siblings, 1 reply; 3+ messages in thread From: Michael Niedermayer @ 2025-08-06 22:05 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 2854 bytes --] Hi On Tue, Aug 05, 2025 at 02:52:28PM -0700, Dale Curtis wrote: > Don't silently skip errors when AV_EF_EXPLODE is specified. This can > lead to out-of-bound reads with ff_put_h264_chroma_mc4_ssse3() when > small padding is used with the checked bitstream reader. > > Signed-off-by: Dale Curtis <dalecurtis@chromium.org> > h264_refs.c | 7 +++++++ > 1 file changed, 7 insertions(+) > e453856d1d24c3a4c2e42d61acdeff3b09b226da h264_stricter_v1.patch > From 46b2fa1ec0cbd00c4fd3909665608d79760654d0 Mon Sep 17 00:00:00 2001 > From: Dale Curtis <dalecurtis@chromium.org> > Date: Tue, 5 Aug 2025 21:45:19 +0000 > Subject: [PATCH] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE > > Don't silently skip errors when AV_EF_EXPLODE is specified. ok > This can > lead to out-of-bound reads with ff_put_h264_chroma_mc4_ssse3() when > small padding is used with the checked bitstream reader. this sounds a bit fishy > > Signed-off-by: Dale Curtis <dalecurtis@chromium.org> > --- > libavcodec/h264_refs.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c > index 74840e5909..e6e3adf502 100644 > --- a/libavcodec/h264_refs.c > +++ b/libavcodec/h264_refs.c > @@ -370,6 +370,9 @@ int ff_h264_build_ref_list(H264Context *h, H264SliceContext *sl) > i < 0 ? "reference picture missing during reorder\n" : > "mismatching reference\n" > ); > + if (h->avctx->err_recognition & AV_EF_EXPLODE) { > + return AVERROR_INVALIDDATA; > + } indention depth is 4 in ffmpeg consistently > memset(&sl->ref_list[list][index], 0, sizeof(sl->ref_list[0][0])); // FIXME > } else { > for (i = index; i + 1 < sl->ref_count[list]; i++) { > @@ -392,6 +395,10 @@ int ff_h264_build_ref_list(H264Context *h, H264SliceContext *sl) > for (int index = 0; index < sl->ref_count[list]; index++) { > if ( !sl->ref_list[list][index].parent > || (!FIELD_PICTURE(h) && (sl->ref_list[list][index].reference&3) != 3)) { > + if (h->avctx->err_recognition & AV_EF_EXPLODE) { > + av_log(h->avctx, AV_LOG_ERROR, "Missing reference picture\n"); > + return AVERROR_INVALIDDATA; > + } > av_log(h->avctx, AV_LOG_ERROR, "Missing reference picture, default is %d\n", h->default_ref[list].poc); the error out can be after this av_log() avoiding the 2nd av_log() in the if() thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I am the wisest man alive, for I know one thing, and that is that I know nothing. -- Socrates [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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". ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [FFmpeg-devel] [h264] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE 2025-08-06 22:05 ` Michael Niedermayer @ 2025-08-07 21:20 ` Dale Curtis 0 siblings, 0 replies; 3+ messages in thread From: Dale Curtis @ 2025-08-07 21:20 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1: Type: text/plain, Size: 3695 bytes --] On Wed, Aug 6, 2025 at 3:05 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > Hi > > On Tue, Aug 05, 2025 at 02:52:28PM -0700, Dale Curtis wrote: > > Don't silently skip errors when AV_EF_EXPLODE is specified. This can > > lead to out-of-bound reads with ff_put_h264_chroma_mc4_ssse3() when > > small padding is used with the checked bitstream reader. > > > > Signed-off-by: Dale Curtis <dalecurtis@chromium.org> > > > h264_refs.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > e453856d1d24c3a4c2e42d61acdeff3b09b226da h264_stricter_v1.patch > > From 46b2fa1ec0cbd00c4fd3909665608d79760654d0 Mon Sep 17 00:00:00 2001 > > From: Dale Curtis <dalecurtis@chromium.org> > > Date: Tue, 5 Aug 2025 21:45:19 +0000 > > Subject: [PATCH] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE > > > > > Don't silently skip errors when AV_EF_EXPLODE is specified. > > ok > > > > This can > > lead to out-of-bound reads with ff_put_h264_chroma_mc4_ssse3() when > > small padding is used with the checked bitstream reader. > > this sounds a bit fishy > I've sent you some details privately about how we end up here. I can drop this from the patch set if you prefer. > > > > > > Signed-off-by: Dale Curtis <dalecurtis@chromium.org> > > --- > > libavcodec/h264_refs.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c > > index 74840e5909..e6e3adf502 100644 > > --- a/libavcodec/h264_refs.c > > +++ b/libavcodec/h264_refs.c > > @@ -370,6 +370,9 @@ int ff_h264_build_ref_list(H264Context *h, > H264SliceContext *sl) > > i < 0 ? "reference picture missing during > reorder\n" : > > "mismatching reference\n" > > ); > > + if (h->avctx->err_recognition & AV_EF_EXPLODE) { > > + return AVERROR_INVALIDDATA; > > + } > > indention depth is 4 in ffmpeg consistently > Done. > > > > memset(&sl->ref_list[list][index], 0, > sizeof(sl->ref_list[0][0])); // FIXME > > } else { > > for (i = index; i + 1 < sl->ref_count[list]; i++) { > > @@ -392,6 +395,10 @@ int ff_h264_build_ref_list(H264Context *h, > H264SliceContext *sl) > > for (int index = 0; index < sl->ref_count[list]; index++) { > > if ( !sl->ref_list[list][index].parent > > || (!FIELD_PICTURE(h) && > (sl->ref_list[list][index].reference&3) != 3)) { > > + if (h->avctx->err_recognition & AV_EF_EXPLODE) { > > + av_log(h->avctx, AV_LOG_ERROR, "Missing reference > picture\n"); > > + return AVERROR_INVALIDDATA; > > + } > > av_log(h->avctx, AV_LOG_ERROR, "Missing reference > picture, default is %d\n", h->default_ref[list].poc); > > the error out can be after this av_log() avoiding the 2nd av_log() in the > if() > I didn't make this change, but can if you prefer. The existing log implies a default is set, but the new log simply indicates the reference picture missing is a fatal error. > > thx > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > I am the wisest man alive, for I know one thing, and that is that I know > nothing. -- Socrates > _______________________________________________ > 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". > [-- Attachment #2: h264_stricter_v2.patch --] [-- Type: application/octet-stream, Size: 2015 bytes --] [-- Attachment #3: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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". ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-07 21:20 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-08-05 21:52 [FFmpeg-devel] [h264] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE Dale Curtis 2025-08-06 22:05 ` Michael Niedermayer 2025-08-07 21:20 ` Dale Curtis
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git