[FFmpeg-devel] [h264] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE

[FFmpeg-devel] [h264] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE

[Dale Curtis]

[-- 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".

Re: [FFmpeg-devel] [h264] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE

[Michael Niedermayer]

[-- 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".

Re: [FFmpeg-devel] [h264] Make ff_h264_build_ref_list stricter with AV_EF_EXPLODE

[Dale Curtis]

[-- 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".