| Message ID | 20220312235227.19626-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/vp9_superframe_split_bsf: Check in size | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 3/12/2022 8:52 PM, Michael Niedermayer wrote: > Fixes: Out of array read > Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/vp9_superframe_split_bsf.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c > index ed0444561a..6af555c078 100644 > --- a/libavcodec/vp9_superframe_split_bsf.c > +++ b/libavcodec/vp9_superframe_split_bsf.c > @@ -51,6 +51,11 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) > return ret; > in = s->buffer_pkt; > > + if (in->size == 0) { !in->size > + ret = AVERROR_INVALIDDATA; > + goto fail; > + } > + > marker = in->data[in->size - 1]; Do we want to abort on in->data && !in->size, or just pass the packet through? I'm partial to the latter, so it would mean initializing marker to 0 and check instead for in->size before setting marker, so the check below fails and the packet is just passed through. Not sure what others think about it. > if ((marker & 0xe0) == 0xc0) { > int length_size = 1 + ((marker >> 3) & 0x3);
On Sun, Mar 13, 2022 at 04:03:42PM -0300, James Almer wrote: > > > On 3/12/2022 8:52 PM, Michael Niedermayer wrote: > > Fixes: Out of array read > > Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/vp9_superframe_split_bsf.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c > > index ed0444561a..6af555c078 100644 > > --- a/libavcodec/vp9_superframe_split_bsf.c > > +++ b/libavcodec/vp9_superframe_split_bsf.c > > @@ -51,6 +51,11 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) > > return ret; > > in = s->buffer_pkt; > > + if (in->size == 0) { > > !in->size I favor checking "== 0" when its about the value 0 and !X when its about something being not set / not allocated. but i can change it if you prefer thx [...]
On 3/14/2022 11:04 AM, Michael Niedermayer wrote: > On Sun, Mar 13, 2022 at 04:03:42PM -0300, James Almer wrote: >> >> >> On 3/12/2022 8:52 PM, Michael Niedermayer wrote: >>> Fixes: Out of array read >>> Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/vp9_superframe_split_bsf.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c >>> index ed0444561a..6af555c078 100644 >>> --- a/libavcodec/vp9_superframe_split_bsf.c >>> +++ b/libavcodec/vp9_superframe_split_bsf.c >>> @@ -51,6 +51,11 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) >>> return ret; >>> in = s->buffer_pkt; >>> + if (in->size == 0) { >> >> !in->size > > I favor checking "== 0" when its about the value 0 and !X when its about > something being not set / not allocated. > but i can change it if you prefer > > thx I suggested it for the sake of consistence. Most AVPacket.size checks use !X to check for 0.
On Mon, Mar 14, 2022 at 11:07:38AM -0300, James Almer wrote: > > > On 3/14/2022 11:04 AM, Michael Niedermayer wrote: > > On Sun, Mar 13, 2022 at 04:03:42PM -0300, James Almer wrote: > > > > > > > > > On 3/12/2022 8:52 PM, Michael Niedermayer wrote: > > > > Fixes: Out of array read > > > > Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/vp9_superframe_split_bsf.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c > > > > index ed0444561a..6af555c078 100644 > > > > --- a/libavcodec/vp9_superframe_split_bsf.c > > > > +++ b/libavcodec/vp9_superframe_split_bsf.c > > > > @@ -51,6 +51,11 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) > > > > return ret; > > > > in = s->buffer_pkt; > > > > + if (in->size == 0) { > > > > > > !in->size > > > > I favor checking "== 0" when its about the value 0 and !X when its about > > something being not set / not allocated. > > but i can change it if you prefer > > > > thx > > I suggested it for the sake of consistence. Most AVPacket.size checks use !X > to check for 0. changed thx [...]
On Sun, Mar 13, 2022 at 04:03:42PM -0300, James Almer wrote: > > > On 3/12/2022 8:52 PM, Michael Niedermayer wrote: > > Fixes: Out of array read > > Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/vp9_superframe_split_bsf.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c > > index ed0444561a..6af555c078 100644 > > --- a/libavcodec/vp9_superframe_split_bsf.c > > +++ b/libavcodec/vp9_superframe_split_bsf.c > > @@ -51,6 +51,11 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) > > return ret; > > in = s->buffer_pkt; > > + if (in->size == 0) { > > !in->size > > > + ret = AVERROR_INVALIDDATA; > > + goto fail; > > + } > > + > > marker = in->data[in->size - 1]; > > Do we want to abort on in->data && !in->size, or just pass the packet > through? > I'm partial to the latter, so it would mean initializing marker to 0 and > check instead for in->size before setting marker, so the check below fails > and the packet is just passed through. > > Not sure what others think about it. I have no opinion on this. I just want the bug fixed. As both andreas and ossfuzz noticed this affects other filters too. not sure the passthough makes sense for all them, i will just dumbly post a passthrough based patch for the other one ossfuzz found too but maybe erroring out as in andreas patch makes more sense thx [...]
diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c index ed0444561a..6af555c078 100644 --- a/libavcodec/vp9_superframe_split_bsf.c +++ b/libavcodec/vp9_superframe_split_bsf.c @@ -51,6 +51,11 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) return ret; in = s->buffer_pkt; + if (in->size == 0) { + ret = AVERROR_INVALIDDATA; + goto fail; + } + marker = in->data[in->size - 1]; if ((marker & 0xe0) == 0xc0) { int length_size = 1 + ((marker >> 3) & 0x3);
Fixes: Out of array read Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vp9_superframe_split_bsf.c | 5 +++++ 1 file changed, 5 insertions(+)