diff mbox series

[FFmpeg-devel,7/7] avcodec/cri: check for available input in unpack_10bit()

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
Related show

Checks

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

Commit Message

Michael Niedermayer Nov. 9, 2020, 11:04 p.m. UTC
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(-)

Comments

Andreas Rheinhardt Nov. 10, 2020, 12:46 a.m. UTC | #1
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
Michael Niedermayer Nov. 10, 2020, 4:17 p.m. UTC | #2
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

[...]
Michael Niedermayer Jan. 21, 2021, 6:53 p.m. UTC | #3
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

[...]
Paul B Mahol Jan. 21, 2021, 10:26 p.m. UTC | #4
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".
Michael Niedermayer Jan. 22, 2021, 12:19 p.m. UTC | #5
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 mbox series

Patch

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) {