| Message ID | 20240623230137.1749178-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avcodec/aac/aacdec_usac: Test ac in usac | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting Michael Niedermayer (2024-06-24 01:01:34) > The slice code detects changes by comparing the pps index. > That way the code cannot detect changes if the sets can change. > > Fixes: out of array access > Fixes: 69585/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6389604524556288 > Fixes: 69601/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5069068613255168 > Fixes: 69621/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6187334182174720 > > 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 | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c > index 1d2e53afc32..d68d454537a 100644 > --- a/libavcodec/hevc/hevcdec.c > +++ b/libavcodec/hevc/hevcdec.c > @@ -3221,17 +3221,20 @@ static int decode_nal_unit(HEVCContext *s, const H2645NAL *nal) > ret = ff_hevc_decode_nal_vps(&gb, s->avctx, &s->ps); > if (ret < 0) > goto fail; > + s->sh.pps_id = -1; This is backwards. If the problem is that the current check does not detect the change, then the check should be fixed.
On Tue, Jun 25, 2024 at 10:59:14AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2024-06-24 01:01:34) > > The slice code detects changes by comparing the pps index. > > That way the code cannot detect changes if the sets can change. > > > > Fixes: out of array access > > Fixes: 69585/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6389604524556288 > > Fixes: 69601/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5069068613255168 > > Fixes: 69621/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6187334182174720 > > > > 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 | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c > > index 1d2e53afc32..d68d454537a 100644 > > --- a/libavcodec/hevc/hevcdec.c > > +++ b/libavcodec/hevc/hevcdec.c > > @@ -3221,17 +3221,20 @@ static int decode_nal_unit(HEVCContext *s, const H2645NAL *nal) > > ret = ff_hevc_decode_nal_vps(&gb, s->avctx, &s->ps); > > if (ret < 0) > > goto fail; > > + s->sh.pps_id = -1; > > This is backwards. If the problem is that the current check does not > detect the change, then the check should be fixed. The specification explcitly doesnt allow the active *PS to change "Any PPS NAL unit containing the value of pps_pic_parameter_set_id for the active PPS RBSP for a coded picture (and consequently for the layer containing the coded picture) shall have the same content as that of the active PPS RBSP for the coded picture, unless it follows the last VCL NAL unit of the coded picture and precedes the first VCL NAL unit of another coded picture." "Any SPS NAL unit with nuh_layer_id equal to 0 containing the value of sps_seq_parameter_set_id for the active SPS RBSP for the base layer for a CVS shall have the same content as that of the active SPS RBSP for the base layer for the CVS, unless it follows the last access unit of the CVS and precedes the first VCL NAL unit and the first SEI NAL unit containing an active parameter sets SEI message (when present) of another CVS." "Any SPS NAL unit with any nuh_layer_id value containing the value of sps_seq_parameter_set_id for the active SPS RBSP for a particular layer shall have the same content as that of the active SPS RBSP for the particular layer unless it follows the access unit auA containing the last coded picture for which the active SPS RBSP for the particular layer is required to be active for the particular layer and precedes the first NAL unit succeeding auA in decoding order that activates an SPS RBSP with the same value of seq_parameter_set_id." So if anything fancy is wanted, the way to go is not to parse it in the first place. Because if it matches then the parsing was a waste of time. If it mismatches, that would be invalid and would just lead to failure anyway But really the primary goal is to fix the out of array access and be easy to backport. I am not trying to fix this in a fancy way. The simpler and more robust the fix is the better. thx [...]
Quoting Michael Niedermayer (2024-06-26 01:27:36) > The specification explcitly doesnt allow the active *PS to change > > "Any PPS NAL unit containing the value of pps_pic_parameter_set_id for the active PPS RBSP for a coded picture (and > consequently for the layer containing the coded picture) shall have the same content as that of the active PPS RBSP for the > coded picture, unless it follows the last VCL NAL unit of the coded picture and precedes the first VCL NAL unit of another > coded picture." > > "Any SPS NAL unit with nuh_layer_id equal to 0 containing the value of sps_seq_parameter_set_id for the active SPS > RBSP for the base layer for a CVS shall have the same content as that of the active SPS RBSP for the base layer for the > CVS, unless it follows the last access unit of the CVS and precedes the first VCL NAL unit and the first SEI NAL unit > containing an active parameter sets SEI message (when present) of another CVS." > > "Any SPS NAL unit with any nuh_layer_id value containing the value of sps_seq_parameter_set_id for the active SPS > RBSP for a particular layer shall have the same content as that of the active SPS RBSP for the particular layer unless it > follows the access unit auA containing the last coded picture for which the active SPS RBSP for the particular layer is > required to be active for the particular layer and precedes the first NAL unit succeeding auA in decoding order that activates > an SPS RBSP with the same value of seq_parameter_set_id." I have no idea why you're quoting the spec at me, I know perfectly well the PPS cannot change between slices. My point is this: if the current check for PPS change does not detect it, it should be fixed. Specifically, it should compare actual PPS objects rather than pps_id. > So if anything fancy is wanted, the way to go is not to parse it in the first place. > Because if it matches then the parsing was a waste of time. If it mismatches, that > would be invalid and would just lead to failure anyway > > But really the primary goal is to fix the out of array access and be easy to > backport. I am not trying to fix this in a fancy way. > The simpler and more robust the fix is the better. This "simple and robust" approach of sprinkling random checks all over the place leads to unreadable and unmaintainable code.
diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c index 1d2e53afc32..d68d454537a 100644 --- a/libavcodec/hevc/hevcdec.c +++ b/libavcodec/hevc/hevcdec.c @@ -3221,17 +3221,20 @@ static int decode_nal_unit(HEVCContext *s, const H2645NAL *nal) ret = ff_hevc_decode_nal_vps(&gb, s->avctx, &s->ps); if (ret < 0) goto fail; + s->sh.pps_id = -1; break; case HEVC_NAL_SPS: ret = ff_hevc_decode_nal_sps(&gb, s->avctx, &s->ps, s->apply_defdispwin); if (ret < 0) goto fail; + s->sh.pps_id = -1; break; case HEVC_NAL_PPS: ret = ff_hevc_decode_nal_pps(&gb, s->avctx, &s->ps); if (ret < 0) goto fail; + s->sh.pps_id = -1; break; case HEVC_NAL_SEI_PREFIX: case HEVC_NAL_SEI_SUFFIX:
The slice code detects changes by comparing the pps index. That way the code cannot detect changes if the sets can change. Fixes: out of array access Fixes: 69585/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6389604524556288 Fixes: 69601/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5069068613255168 Fixes: 69621/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6187334182174720 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 | 3 +++ 1 file changed, 3 insertions(+)