| Message ID | 20230601222601.12631-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] tools/target_dec_fuzzer: adjust threshold for RKA | 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 |
Michael Niedermayer: > On error pointers can be left NULL while code later assumes these not to be NULL > > Fixes: NULL pointer dereference > Fixes: 59359/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-6726080594313216 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/cbs_av1.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c > index 8788fee099..7f3f4da2f5 100644 > --- a/libavcodec/cbs_av1.c > +++ b/libavcodec/cbs_av1.c > @@ -1013,8 +1013,10 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx, > case AV1_OBU_METADATA: > { > err = cbs_av1_read_metadata_obu(ctx, &gbc, &obu->obu.metadata); > - if (err < 0) > + if (err < 0) { > + memset(&obu->obu.metadata, 0, sizeof(obu->obu.metadata)); > return err; > + } > } > break; > case AV1_OBU_PADDING: 1. Before 97f4263, the current_obu was reset (and the packet effectively discarded) upon errors from ff_cbs_read_packet(); yet this is no longer true and it seems that the contents of current_obu will be processed in the next call to av1_receive_frame(). This change seems to have been unintentional. 2. The commit message is weird: You claim that it is bad that a pointer is NULL; and then you go on and zero it again. It would be better to claim that the metadata is in an inconsistent state. 3. There is a possibility for inconsistency in cbs_av1_syntax_template, namely if the allocation fails, metadata OBU nevertheless claims to have a payload_size > 0; payload_size should only be set after the allocation succeeded. But this does not seem to be the issue in this testcase. Presumably there are not enough bits left for the itu_t_t35_country_code or its extension? In this case, cbs_av1 did not even make an error. 4. Your fix is dangerous: In case the code were changed so that an error can happen after a successful allocation, your memset would lead to leaks. (The most likely possibility is the addition of a new type of metadata; another way would be for the code to be changed to avoid reading the metadata twice by reallocating (and overallocating) the buffer as needed.) - Andreas
On 6/1/2023 9:28 PM, Andreas Rheinhardt wrote: > 1. Before 97f4263, the current_obu was reset (and the packet effectively > discarded) upon errors from ff_cbs_read_packet(); yet this is no longer > true and it seems that the contents of current_obu will be processed in > the next call to av1_receive_frame(). This change seems to have been > unintentional. I guess I assumed that ff_cbs_read_packet() failing would clear the CodedBitstreamFragment before returning, but if that's not the case then ff_cbs_fragment_reset() should be called. Would something like > diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c > index 5cc5d87c64..d1a0f6eaa2 100644 > --- a/libavcodec/av1dec.c > +++ b/libavcodec/av1dec.c > @@ -1461,6 +1461,7 @@ static int av1_receive_frame(AVCodecContext *avctx, AVFrame *frame) > ret = ff_cbs_read_packet(s->cbc, &s->current_obu, s->pkt); > if (ret < 0) { > av_packet_unref(s->pkt); > + ff_cbs_fragment_reset(&s->current_obu); > av_log(avctx, AV_LOG_ERROR, "Failed to read packet.\n"); > return ret; > } Be enough?
On Thu, Jun 01, 2023 at 09:33:14PM -0300, James Almer wrote: > On 6/1/2023 9:28 PM, Andreas Rheinhardt wrote: > > 1. Before 97f4263, the current_obu was reset (and the packet effectively > > discarded) upon errors from ff_cbs_read_packet(); yet this is no longer > > true and it seems that the contents of current_obu will be processed in > > the next call to av1_receive_frame(). This change seems to have been > > unintentional. > > I guess I assumed that ff_cbs_read_packet() failing would clear the > CodedBitstreamFragment before returning, but if that's not the case then > ff_cbs_fragment_reset() should be called. > > Would something like > > > diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c > > index 5cc5d87c64..d1a0f6eaa2 100644 > > --- a/libavcodec/av1dec.c > > +++ b/libavcodec/av1dec.c > > @@ -1461,6 +1461,7 @@ static int av1_receive_frame(AVCodecContext *avctx, AVFrame *frame) > > ret = ff_cbs_read_packet(s->cbc, &s->current_obu, s->pkt); > > if (ret < 0) { > > av_packet_unref(s->pkt); > > + ff_cbs_fragment_reset(&s->current_obu); > > av_log(avctx, AV_LOG_ERROR, "Failed to read packet.\n"); > > return ret; > > } > > Be enough? yes, that solves it and looks much better than my patch thx [...]
On Fri, Jun 02, 2023 at 02:28:24AM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On error pointers can be left NULL while code later assumes these not to be NULL > > > > Fixes: NULL pointer dereference > > Fixes: 59359/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-6726080594313216 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/cbs_av1.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c > > index 8788fee099..7f3f4da2f5 100644 > > --- a/libavcodec/cbs_av1.c > > +++ b/libavcodec/cbs_av1.c > > @@ -1013,8 +1013,10 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx, > > case AV1_OBU_METADATA: > > { > > err = cbs_av1_read_metadata_obu(ctx, &gbc, &obu->obu.metadata); > > - if (err < 0) > > + if (err < 0) { > > + memset(&obu->obu.metadata, 0, sizeof(obu->obu.metadata)); > > return err; > > + } > > } > > break; > > case AV1_OBU_PADDING: > > 1. Before 97f4263, the current_obu was reset (and the packet effectively > discarded) upon errors from ff_cbs_read_packet(); yet this is no longer > true and it seems that the contents of current_obu will be processed in > the next call to av1_receive_frame(). This change seems to have been > unintentional. > 2. The commit message is weird: You claim that it is bad that a pointer > is NULL; and then you go on and zero it again. It would be better to > claim that the metadata is in an inconsistent state. > 3. There is a possibility for inconsistency in cbs_av1_syntax_template, > namely if the allocation fails, metadata OBU nevertheless claims to have > a payload_size > 0; payload_size should only be set after the allocation > succeeded. But this does not seem to be the issue in this testcase. > Presumably there are not enough bits left for the itu_t_t35_country_code > or its extension? In this case, cbs_av1 did not even make an error. > 4. Your fix is dangerous: In case the code were changed so that an error > can happen after a successful allocation, your memset would lead to > leaks. (The most likely possibility is the addition of a new type of > metadata; another way would be for the code to be changed to avoid > reading the metadata twice by reallocating (and overallocating) the > buffer as needed.) posting a suboptimal fix, if one isnt sure what is the clean way to fix it is a great way to have someone else make a better fix. thx [...]
diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c index 8788fee099..7f3f4da2f5 100644 --- a/libavcodec/cbs_av1.c +++ b/libavcodec/cbs_av1.c @@ -1013,8 +1013,10 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx, case AV1_OBU_METADATA: { err = cbs_av1_read_metadata_obu(ctx, &gbc, &obu->obu.metadata); - if (err < 0) + if (err < 0) { + memset(&obu->obu.metadata, 0, sizeof(obu->obu.metadata)); return err; + } } break; case AV1_OBU_PADDING:
On error pointers can be left NULL while code later assumes these not to be NULL Fixes: NULL pointer dereference Fixes: 59359/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-6726080594313216 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_av1.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)