[FFmpeg-devel] [PATCH v2] avcodec/hevc/hevcdec: Do not allow slices to depend on failed slices

[FFmpeg-devel] [PATCH v2] avcodec/hevc/hevcdec: Do not allow slices to depend on failed slices

[Michael Niedermayer]

An alternative would be to leave the context unchanged on failure of hls_slice_header()

Fixes: out of array access
Fixes: NULL pointer dereference
Fixes: 69584/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5931086299856896
Fixes: 69724/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5104066422702080

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/hevc/hevcdec.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c
index 0e4b26dad3b..80d59ab1916 100644
--- a/libavcodec/hevc/hevcdec.c
+++ b/libavcodec/hevc/hevcdec.c
@@ -621,6 +621,10 @@ static int hls_slice_header(SliceHeader *sh, const HEVCContext *s, GetBitContext
 
         if (pps->dependent_slice_segments_enabled_flag)
             sh->dependent_slice_segment_flag = get_bits1(gb);
+        if (sh->dependent_slice_segment_flag && !s->slice_initialized) {
+            av_log(s->avctx, AV_LOG_ERROR, "dependent slice failed\n");
+            return AVERROR_INVALIDDATA;
+        }
 
         slice_address_length = av_ceil_log2(sps->ctb_width *
                                             sps->ctb_height);
@@ -893,9 +897,6 @@ static int hls_slice_header(SliceHeader *sh, const HEVCContext *s, GetBitContext
         } else {
             sh->slice_loop_filter_across_slices_enabled_flag = pps->seq_loop_filter_across_slices_enabled_flag;
         }
-    } else if (!s->slice_initialized) {
-        av_log(s->avctx, AV_LOG_ERROR, "Independent slice segment missing.\n");
-        return AVERROR_INVALIDDATA;
     }
 
     sh->num_entry_point_offsets = 0;
@@ -3155,8 +3156,11 @@ static int decode_slice(HEVCContext *s, const H2645NAL *nal, GetBitContext *gb)
     int ret;
 
     ret = hls_slice_header(&s->sh, s, gb);
-    if (ret < 0)
+    if (ret < 0) {
+        //The code is not capable to rewind from an error, the state now is inconsistant so we cannot use it on depandant slices
+        s->slice_initialized = 0;
         return ret;
+    }
 
     if ((s->avctx->skip_frame >= AVDISCARD_BIDIR && s->sh.slice_type == HEVC_SLICE_B) ||
         (s->avctx->skip_frame >= AVDISCARD_NONINTRA && s->sh.slice_type != HEVC_SLICE_I) ||
-- 
2.45.2

_______________________________________________
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] [PATCH v2] avcodec/hevc/hevcdec: Do not allow slices to depend on failed slices

[Anton Khirnov]

Quoting Michael Niedermayer (2024-07-15 16:48:25)
> An alternative would be to leave the context unchanged on failure of hls_slice_header()
> 
> Fixes: out of array access
> Fixes: NULL pointer dereference
> Fixes: 69584/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5931086299856896
> Fixes: 69724/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5104066422702080
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hevc/hevcdec.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c
> index 0e4b26dad3b..80d59ab1916 100644
> --- a/libavcodec/hevc/hevcdec.c
> +++ b/libavcodec/hevc/hevcdec.c
> @@ -621,6 +621,10 @@ static int hls_slice_header(SliceHeader *sh, const HEVCContext *s, GetBitContext
>  
>          if (pps->dependent_slice_segments_enabled_flag)
>              sh->dependent_slice_segment_flag = get_bits1(gb);
> +        if (sh->dependent_slice_segment_flag && !s->slice_initialized) {
> +            av_log(s->avctx, AV_LOG_ERROR, "dependent slice failed\n");

The new error message seems worse than the old one. A slice is a passive
object, "slice failed" makes no sense.

> @@ -3155,8 +3156,11 @@ static int decode_slice(HEVCContext *s, const H2645NAL *nal, GetBitContext *gb)
>      int ret;
>  
>      ret = hls_slice_header(&s->sh, s, gb);
> -    if (ret < 0)
> +    if (ret < 0) {
> +        //The code is not capable to rewind from an error, the state now is inconsistant so we cannot use it on depandant slices
                                                                                        ^                                ^
                                                                                        e                                e
Also I'd drop everything before the comma, why mention rewinding that is
not implemented and may not be a good idea anyway.

Otherwise patch LGTM.

-- 
Anton Khirnov
_______________________________________________
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] [PATCH v2] avcodec/hevc/hevcdec: Do not allow slices to depend on failed slices

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1528 bytes --]

On Wed, Jul 17, 2024 at 08:42:41AM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-07-15 16:48:25)
[...]
> > @@ -3155,8 +3156,11 @@ static int decode_slice(HEVCContext *s, const H2645NAL *nal, GetBitContext *gb)
> >      int ret;
> >  
> >      ret = hls_slice_header(&s->sh, s, gb);
> > -    if (ret < 0)
> > +    if (ret < 0) {
> > +        //The code is not capable to rewind from an error, the state now is inconsistant so we cannot use it on depandant slices
>                                                                                         ^                                ^
>                                                                                         e                                e
> Also I'd drop everything before the comma, why mention rewinding that is
> not implemented and may not be a good idea anyway.

It seems we disagree on some things.
A failing function should not corrupt the state. We also dont do that
in other cases like SPS/PPS.
This is also unexpected, so it should be documented.

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued

[-- 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] [PATCH v2] avcodec/hevc/hevcdec: Do not allow slices to depend on failed slices

[Anton Khirnov]

Quoting Michael Niedermayer (2024-07-20 02:08:28)
> On Wed, Jul 17, 2024 at 08:42:41AM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-07-15 16:48:25)
> [...]
> > > @@ -3155,8 +3156,11 @@ static int decode_slice(HEVCContext *s, const H2645NAL *nal, GetBitContext *gb)
> > >      int ret;
> > >  
> > >      ret = hls_slice_header(&s->sh, s, gb);
> > > -    if (ret < 0)
> > > +    if (ret < 0) {
> > > +        //The code is not capable to rewind from an error, the state now is inconsistant so we cannot use it on depandant slices
> >                                                                                         ^                                ^
> >                                                                                         e                                e
> > Also I'd drop everything before the comma, why mention rewinding that is
> > not implemented and may not be a good idea anyway.
> 
> It seems we disagree on some things.
> A failing function should not corrupt the state. We also dont do that
> in other cases like SPS/PPS.
> This is also unexpected, so it should be documented.

In my view, the root problem is not that the state is corrupted, but
that the independent slice segment is missing. No rewinding you can do
will conjure it out of nowhere, and it's not at all clear to me that
using some random previous slice segment's data is the right thing to
do.

-- 
Anton Khirnov
_______________________________________________
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] [PATCH v2] avcodec/hevc/hevcdec: Do not allow slices to depend on failed slices

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1931 bytes --]

On Sat, Jul 20, 2024 at 08:11:02AM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-07-20 02:08:28)
> > On Wed, Jul 17, 2024 at 08:42:41AM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2024-07-15 16:48:25)
> > [...]
> > > > @@ -3155,8 +3156,11 @@ static int decode_slice(HEVCContext *s, const H2645NAL *nal, GetBitContext *gb)
> > > >      int ret;
> > > >  
> > > >      ret = hls_slice_header(&s->sh, s, gb);
> > > > -    if (ret < 0)
> > > > +    if (ret < 0) {
> > > > +        //The code is not capable to rewind from an error, the state now is inconsistant so we cannot use it on depandant slices
> > >                                                                                         ^                                ^
> > >                                                                                         e                                e
> > > Also I'd drop everything before the comma, why mention rewinding that is
> > > not implemented and may not be a good idea anyway.
> > 
> > It seems we disagree on some things.
> > A failing function should not corrupt the state. We also dont do that
> > in other cases like SPS/PPS.
> > This is also unexpected, so it should be documented.
> 
> In my view, the root problem is not that the state is corrupted, but
> that the independent slice segment is missing. No rewinding you can do
> will conjure it out of nowhere, and it's not at all clear to me that
> using some random previous slice segment's data is the right thing to
> do.

I think it is bad practice to leave inconsistant state and "Just not use it"
because if something either now or in the future does end up using it, that
could result in undeffined or unexpected behavior

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.

[-- 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] [PATCH v2] avcodec/hevc/hevcdec: Do not allow slices to depend on failed slices

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2538 bytes --]

On Wed, Jul 17, 2024 at 08:42:41AM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-07-15 16:48:25)
> > An alternative would be to leave the context unchanged on failure of hls_slice_header()
> > 
> > Fixes: out of array access
> > Fixes: NULL pointer dereference
> > Fixes: 69584/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5931086299856896
> > Fixes: 69724/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5104066422702080
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/hevc/hevcdec.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c
> > index 0e4b26dad3b..80d59ab1916 100644
> > --- a/libavcodec/hevc/hevcdec.c
> > +++ b/libavcodec/hevc/hevcdec.c
> > @@ -621,6 +621,10 @@ static int hls_slice_header(SliceHeader *sh, const HEVCContext *s, GetBitContext
> >  
> >          if (pps->dependent_slice_segments_enabled_flag)
> >              sh->dependent_slice_segment_flag = get_bits1(gb);
> > +        if (sh->dependent_slice_segment_flag && !s->slice_initialized) {
> > +            av_log(s->avctx, AV_LOG_ERROR, "dependent slice failed\n");
> 
> The new error message seems worse than the old one. A slice is a passive
> object, "slice failed" makes no sense.

will leave the error message


> 
> > @@ -3155,8 +3156,11 @@ static int decode_slice(HEVCContext *s, const H2645NAL *nal, GetBitContext *gb)
> >      int ret;
> >  
> >      ret = hls_slice_header(&s->sh, s, gb);
> > -    if (ret < 0)
> > +    if (ret < 0) {
> > +        //The code is not capable to rewind from an error, the state now is inconsistant so we cannot use it on depandant slices
>                                                                                         ^                                ^
>                                                                                         e                                e
> Also I'd drop everything before the comma, why mention rewinding that is
> not implemented and may not be a good idea anyway.

will word it differently


> 
> Otherwise patch LGTM.

will apply

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 

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