| Message ID | 20201109230456.11188-7-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 43c8d3097b8254d08f5413a1934c001327859f47 |
| Headers | show |
| Series | [FFmpeg-devel,1/7] avformat/sbgdec: Check that end is not before start | expand |
| Context | Check | Description |
|---|---|---|
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
Michael Niedermayer: > Fixes: Timeout (>20sec -> 56ms) > Fixes: 26995/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CRI_fuzzer-5107217080254464 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/cri.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/cri.c b/libavcodec/cri.c > index dafbc1f1be..9bcd2ebfc6 100644 > --- a/libavcodec/cri.c > +++ b/libavcodec/cri.c > @@ -80,10 +80,13 @@ static void unpack_10bit(GetByteContext *gb, uint16_t *dst, int shift, > int pos = 0; > > while (count > 0) { > - uint32_t a0 = bytestream2_get_le32(gb); > - uint32_t a1 = bytestream2_get_le32(gb); > - uint32_t a2 = bytestream2_get_le32(gb); > - uint32_t a3 = bytestream2_get_le32(gb); > + uint32_t a0, a1,a2,a3; > + if (bytestream2_get_bytes_left(gb) < 4) > + break; > + a0 = bytestream2_get_le32(gb); > + a1 = bytestream2_get_le32(gb); > + a2 = bytestream2_get_le32(gb); > + a3 = bytestream2_get_le32(gb); > dst[pos] = (((a0 >> 1) & 0xE00) | (a0 & 0x1FF)) << shift; > pos++; > if (pos >= w) { > Wouldn't it make sense to check for 16 bytes to be left given that that's the amount that is read immediately afterwards? And if you check for this, you could just use bytestream2_get_le32u(). - Andreas
On Tue, Nov 10, 2020 at 01:46:10AM +0100, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Fixes: Timeout (>20sec -> 56ms) > > Fixes: 26995/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CRI_fuzzer-5107217080254464 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/cri.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/libavcodec/cri.c b/libavcodec/cri.c > > index dafbc1f1be..9bcd2ebfc6 100644 > > --- a/libavcodec/cri.c > > +++ b/libavcodec/cri.c > > @@ -80,10 +80,13 @@ static void unpack_10bit(GetByteContext *gb, uint16_t *dst, int shift, > > int pos = 0; > > > > while (count > 0) { > > - uint32_t a0 = bytestream2_get_le32(gb); > > - uint32_t a1 = bytestream2_get_le32(gb); > > - uint32_t a2 = bytestream2_get_le32(gb); > > - uint32_t a3 = bytestream2_get_le32(gb); > > + uint32_t a0, a1,a2,a3; > > + if (bytestream2_get_bytes_left(gb) < 4) > > + break; > > + a0 = bytestream2_get_le32(gb); > > + a1 = bytestream2_get_le32(gb); > > + a2 = bytestream2_get_le32(gb); > > + a3 = bytestream2_get_le32(gb); > > dst[pos] = (((a0 >> 1) & 0xE00) | (a0 & 0x1FF)) << shift; > > pos++; > > if (pos >= w) { > > > Wouldn't it make sense to check for 16 bytes to be left given that > that's the amount that is read immediately afterwards? And if you check > for this, you could just use bytestream2_get_le32u(). the code can break out before using all 4 so i felt it was more cautious to check only for the first thx [...]
On Tue, Nov 10, 2020 at 05:17:40PM +0100, Michael Niedermayer wrote: > On Tue, Nov 10, 2020 at 01:46:10AM +0100, Andreas Rheinhardt wrote: > > Michael Niedermayer: > > > Fixes: Timeout (>20sec -> 56ms) > > > Fixes: 26995/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CRI_fuzzer-5107217080254464 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/cri.c | 11 +++++++---- > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > > > diff --git a/libavcodec/cri.c b/libavcodec/cri.c > > > index dafbc1f1be..9bcd2ebfc6 100644 > > > --- a/libavcodec/cri.c > > > +++ b/libavcodec/cri.c > > > @@ -80,10 +80,13 @@ static void unpack_10bit(GetByteContext *gb, uint16_t *dst, int shift, > > > int pos = 0; > > > > > > while (count > 0) { > > > - uint32_t a0 = bytestream2_get_le32(gb); > > > - uint32_t a1 = bytestream2_get_le32(gb); > > > - uint32_t a2 = bytestream2_get_le32(gb); > > > - uint32_t a3 = bytestream2_get_le32(gb); > > > + uint32_t a0, a1,a2,a3; > > > + if (bytestream2_get_bytes_left(gb) < 4) > > > + break; > > > + a0 = bytestream2_get_le32(gb); > > > + a1 = bytestream2_get_le32(gb); > > > + a2 = bytestream2_get_le32(gb); > > > + a3 = bytestream2_get_le32(gb); > > > dst[pos] = (((a0 >> 1) & 0xE00) | (a0 & 0x1FF)) << shift; > > > pos++; > > > if (pos >= w) { > > > > > Wouldn't it make sense to check for 16 bytes to be left given that > > that's the amount that is read immediately afterwards? And if you check > > for this, you could just use bytestream2_get_le32u(). > > the code can break out before using all 4 so i felt it was more > cautious to check only for the first will apply [...]
On Tue, Nov 10, 2020 at 12:15 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: Timeout (>20sec -> 56ms) > Fixes: > 26995/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CRI_fuzzer-5107217080254464 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by > <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>: > Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/cri.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/cri.c b/libavcodec/cri.c > index dafbc1f1be..9bcd2ebfc6 100644 > --- a/libavcodec/cri.c > +++ b/libavcodec/cri.c > @@ -80,10 +80,13 @@ static void unpack_10bit(GetByteContext *gb, uint16_t > *dst, int shift, > int pos = 0; > > while (count > 0) { > - uint32_t a0 = bytestream2_get_le32(gb); > - uint32_t a1 = bytestream2_get_le32(gb); > - uint32_t a2 = bytestream2_get_le32(gb); > - uint32_t a3 = bytestream2_get_le32(gb); > + uint32_t a0, a1,a2,a3; > Why style issue is very bad here? > + if (bytestream2_get_bytes_left(gb) < 4) > This should be 16, not 4. > + break; > + a0 = bytestream2_get_le32(gb); > + a1 = bytestream2_get_le32(gb); > + a2 = bytestream2_get_le32(gb); > + a3 = bytestream2_get_le32(gb); > dst[pos] = (((a0 >> 1) & 0xE00) | (a0 & 0x1FF)) << shift; > pos++; > if (pos >= w) { > -- > 2.17.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Thu, Jan 21, 2021 at 11:26:57PM +0100, Paul B Mahol wrote: > On Tue, Nov 10, 2020 at 12:15 AM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > Fixes: Timeout (>20sec -> 56ms) > > Fixes: > > 26995/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CRI_fuzzer-5107217080254464 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by > > <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>: > > Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/cri.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/libavcodec/cri.c b/libavcodec/cri.c > > index dafbc1f1be..9bcd2ebfc6 100644 > > --- a/libavcodec/cri.c > > +++ b/libavcodec/cri.c > > @@ -80,10 +80,13 @@ static void unpack_10bit(GetByteContext *gb, uint16_t > > *dst, int shift, > > int pos = 0; > > > > while (count > 0) { > > - uint32_t a0 = bytestream2_get_le32(gb); > > - uint32_t a1 = bytestream2_get_le32(gb); > > - uint32_t a2 = bytestream2_get_le32(gb); > > - uint32_t a3 = bytestream2_get_le32(gb); > > + uint32_t a0, a1,a2,a3; > > > > Why style issue is very bad here? > > > > + if (bytestream2_get_bytes_left(gb) < 4) > > > > This should be 16, not 4. only a0 is unconditionally used. a1 only if count is bigger than 2 a2 only if count is bigger than 4 and so forth so breaking out only if used input is unavailable was done in the patch. I can change it to 16 but that may break some files as it fails on the input lacking unused data Thanks [...]
diff --git a/libavcodec/cri.c b/libavcodec/cri.c index dafbc1f1be..9bcd2ebfc6 100644 --- a/libavcodec/cri.c +++ b/libavcodec/cri.c @@ -80,10 +80,13 @@ static void unpack_10bit(GetByteContext *gb, uint16_t *dst, int shift, int pos = 0; while (count > 0) { - uint32_t a0 = bytestream2_get_le32(gb); - uint32_t a1 = bytestream2_get_le32(gb); - uint32_t a2 = bytestream2_get_le32(gb); - uint32_t a3 = bytestream2_get_le32(gb); + uint32_t a0, a1,a2,a3; + if (bytestream2_get_bytes_left(gb) < 4) + break; + a0 = bytestream2_get_le32(gb); + a1 = bytestream2_get_le32(gb); + a2 = bytestream2_get_le32(gb); + a3 = bytestream2_get_le32(gb); dst[pos] = (((a0 >> 1) & 0xE00) | (a0 & 0x1FF)) << shift; pos++; if (pos >= w) {
Fixes: Timeout (>20sec -> 56ms) Fixes: 26995/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CRI_fuzzer-5107217080254464 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cri.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)