| Message ID | 20191208213120.15190-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
On 12/8/2019 6:31 PM, Michael Niedermayer wrote: > This allows writing empty slices Are empty slices valid in mpeg2 to begin with? Or is that the result of invalid/truncated data exclusively? The code in cbs_mpeg2_write_slice() is run only when slice->data is not NULL, which after a cursory look at cbs_mpeg2_read_slice() it looks like is always the case (It will always have a pointer, even if it points to the very end of the buffer). In contrast, slice->data_size can be zero, so maybe the check here should be slice->data_size != 0 instead, which should fix this assertion failure without changing the it or what data the module will accept. > > Fixes: Assertion slice->data_bit_start >= 0 && slice->data_size > slice->data_bit_start / 8 failed at libavcodec/cbs_mpeg2.c:340 > Fixes: 19280/clusterfuzz-testcase-minimized-ffmpeg_BSF_MPEG2_METADATA_fuzzer-5632206173372416 > > 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_mpeg2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c > index 13d871cc89..b5286537db 100644 > --- a/libavcodec/cbs_mpeg2.c > +++ b/libavcodec/cbs_mpeg2.c > @@ -337,7 +337,7 @@ static int cbs_mpeg2_write_slice(CodedBitstreamContext *ctx, > uint8_t *pos = slice->data + slice->data_bit_start / 8; > > av_assert0(slice->data_bit_start >= 0 && > - slice->data_size > slice->data_bit_start / 8); > + slice->data_size * 8LL >= slice->data_bit_start); > > if (slice->data_size * 8 + 8 > put_bits_left(pbc)) > return AVERROR(ENOSPC); >
On Mon, Dec 9, 2019 at 2:54 AM James Almer <jamrial@gmail.com> wrote: > On 12/8/2019 6:31 PM, Michael Niedermayer wrote: > > This allows writing empty slices > > Are empty slices valid in mpeg2 to begin with? Or is that the result of > invalid/truncated data exclusively? > > It's invalid data, unless data partitioning (which we do not and IMO should never support) is in use. Data partitioning allows to split the bitstream into two that can be sent independently (and over channels of different reliability) and it allows the slice in the base layer to only contain everything excluding the macroblocks (i.e. everything that we put into the slice header). But if data partitioning is not in use, there has to be a macroblock after the extra_information_slice and a macroblock consists of more than zero bits; in fact, there always has to be a bit set to 1 among the first eight bits (which is not meant to imply that every macroblock needs to contain eight bits at all). I will add a check to error out if slices without data beyond the slice header are encountered upon reading. And also similar patches for cbs_h2645. - Andreas
On Mon, Dec 09, 2019 at 12:52:45PM +0100, Andreas Rheinhardt wrote: > On Mon, Dec 9, 2019 at 2:54 AM James Almer <jamrial@gmail.com> wrote: > > > On 12/8/2019 6:31 PM, Michael Niedermayer wrote: > > > This allows writing empty slices > > > > Are empty slices valid in mpeg2 to begin with? Or is that the result of > > invalid/truncated data exclusively? > > > > It's invalid data, unless data partitioning (which we do not and IMO > should never support) is in use. Data partitioning allows to split the > bitstream into two that can be sent independently (and over channels of > different reliability) and it allows the slice in the base layer to only > contain everything excluding the macroblocks (i.e. everything that we put > into the slice header). > > But if data partitioning is not in use, there has to be a macroblock after > the extra_information_slice and a macroblock consists of more than zero > bits; in fact, there always has to be a bit set to 1 among the first eight > bits (which is not meant to imply that every macroblock needs to contain > eight bits at all). > > I will add a check to error out if slices without data beyond the slice > header are encountered upon reading. And also similar patches for cbs_h2645. well, we can be more picky and error out yes. Thats primarly a question what to do when the input isnt strictly valid, do we support maintaining all parts in their invalid form or do we do something else like droping parts that arent valid. Thanks [...]
diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index 13d871cc89..b5286537db 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -337,7 +337,7 @@ static int cbs_mpeg2_write_slice(CodedBitstreamContext *ctx, uint8_t *pos = slice->data + slice->data_bit_start / 8; av_assert0(slice->data_bit_start >= 0 && - slice->data_size > slice->data_bit_start / 8); + slice->data_size * 8LL >= slice->data_bit_start); if (slice->data_size * 8 + 8 > put_bits_left(pbc)) return AVERROR(ENOSPC);
This allows writing empty slices Fixes: Assertion slice->data_bit_start >= 0 && slice->data_size > slice->data_bit_start / 8 failed at libavcodec/cbs_mpeg2.c:340 Fixes: 19280/clusterfuzz-testcase-minimized-ffmpeg_BSF_MPEG2_METADATA_fuzzer-5632206173372416 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_mpeg2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)