| Message ID | 20240715144825.3652038-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] avcodec/hevc/hevcdec: Do not allow slices to depend on failed slices | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
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 [...]
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.
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 [...]
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 [...]
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) ||
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(-)