diff mbox

[FFmpeg-devel,3/3] avcodec/mlpdec: Check quant_step_size against huff_lsbs

Message ID 20170520210104.4943-3-michael@niedermayer.cc
State Accepted
Commit 361e0310d95bf2a0377f168518d1135ae15ca3f8
Headers show

Commit Message

Michael Niedermayer May 20, 2017, 9:01 p.m. UTC 
This reorders the operations so as to avoid computations with the above arguments
before they have been initialized.
Fixes part of 1708/clusterfuzz-testcase-minimized-5035111957397504

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/mlpdec.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

Comments

wm4 May 21, 2017, 11:42 a.m. UTC | #1 
On Sat, 20 May 2017 23:01:04 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> This reorders the operations so as to avoid computations with the above arguments
> before they have been initialized.
> Fixes part of 1708/clusterfuzz-testcase-minimized-5035111957397504
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/mlpdec.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/libavcodec/mlpdec.c b/libavcodec/mlpdec.c
> index c0a23c5f0d..11be380d27 100644
> --- a/libavcodec/mlpdec.c
> +++ b/libavcodec/mlpdec.c
> @@ -825,8 +825,6 @@ static int read_channel_params(MLPDecodeContext *m, unsigned int substr,
>          return AVERROR_INVALIDDATA;
>      }
>  
> -    cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
> -
>      return 0;
>  }
>  
> @@ -838,7 +836,8 @@ static int read_decoding_params(MLPDecodeContext *m, GetBitContext *gbp,
>  {
>      SubStream *s = &m->substream[substr];
>      unsigned int ch;
> -    int ret;
> +    int ret = 0;
> +    unsigned recompute_sho = 0;
>  
>      if (s->param_presence_flags & PARAM_PRESENCE)
>          if (get_bits1(gbp))
> @@ -878,19 +877,36 @@ static int read_decoding_params(MLPDecodeContext *m, GetBitContext *gbp,
>      if (s->param_presence_flags & PARAM_QUANTSTEP)
>          if (get_bits1(gbp))
>              for (ch = 0; ch <= s->max_channel; ch++) {
> -                ChannelParams *cp = &s->channel_params[ch];
> -
>                  s->quant_step_size[ch] = get_bits(gbp, 4);
>  
> -                cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
> +                recompute_sho |= 1<<ch;
>              }
>  
>      for (ch = s->min_channel; ch <= s->max_channel; ch++)
> -        if (get_bits1(gbp))
> +        if (get_bits1(gbp)) {
> +            recompute_sho |= 1<<ch;
>              if ((ret = read_channel_params(m, substr, gbp, ch)) < 0)
> -                return ret;
> +                goto fail;
> +        }
>  
> -    return 0;
> +


> +fail:
> +    for (ch = 0; ch <= s->max_channel; ch++) {
> +        if (recompute_sho & (1<<ch)) {
> +            ChannelParams *cp = &s->channel_params[ch];
> +
> +            if (cp->codebook > 0 && cp->huff_lsbs < s->quant_step_size[ch]) {
> +                if (ret >= 0) {
> +                    av_log(m->avctx, AV_LOG_ERROR, "quant_step_size larger than huff_lsbs\n");
> +                    ret = AVERROR_INVALIDDATA;
> +                }
> +                s->quant_step_size[ch] = 0;
> +            }
> +
> +            cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
> +        }
> +    }
> +    return ret;

What's all this stuff for?

>  }
>  
>  #define MSB_MASK(bits)  (-1u << (bits))
Michael Niedermayer May 22, 2017, 3:36 p.m. UTC | #2 
On Sun, May 21, 2017 at 01:42:18PM +0200, wm4 wrote:
> On Sat, 20 May 2017 23:01:04 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > This reorders the operations so as to avoid computations with the above arguments
> > before they have been initialized.
> > Fixes part of 1708/clusterfuzz-testcase-minimized-5035111957397504
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/mlpdec.c | 34 +++++++++++++++++++++++++---------
> >  1 file changed, 25 insertions(+), 9 deletions(-)
> > 
> > diff --git a/libavcodec/mlpdec.c b/libavcodec/mlpdec.c
> > index c0a23c5f0d..11be380d27 100644
> > --- a/libavcodec/mlpdec.c
> > +++ b/libavcodec/mlpdec.c
> > @@ -825,8 +825,6 @@ static int read_channel_params(MLPDecodeContext *m, unsigned int substr,
> >          return AVERROR_INVALIDDATA;
> >      }
> >  
> > -    cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
> > -
> >      return 0;
> >  }
> >  
> > @@ -838,7 +836,8 @@ static int read_decoding_params(MLPDecodeContext *m, GetBitContext *gbp,
> >  {
> >      SubStream *s = &m->substream[substr];
> >      unsigned int ch;
> > -    int ret;
> > +    int ret = 0;
> > +    unsigned recompute_sho = 0;
> >  
> >      if (s->param_presence_flags & PARAM_PRESENCE)
> >          if (get_bits1(gbp))
> > @@ -878,19 +877,36 @@ static int read_decoding_params(MLPDecodeContext *m, GetBitContext *gbp,
> >      if (s->param_presence_flags & PARAM_QUANTSTEP)
> >          if (get_bits1(gbp))
> >              for (ch = 0; ch <= s->max_channel; ch++) {
> > -                ChannelParams *cp = &s->channel_params[ch];
> > -
> >                  s->quant_step_size[ch] = get_bits(gbp, 4);
> >  
> > -                cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
> > +                recompute_sho |= 1<<ch;
> >              }
> >  
> >      for (ch = s->min_channel; ch <= s->max_channel; ch++)
> > -        if (get_bits1(gbp))
> > +        if (get_bits1(gbp)) {
> > +            recompute_sho |= 1<<ch;
> >              if ((ret = read_channel_params(m, substr, gbp, ch)) < 0)
> > -                return ret;
> > +                goto fail;
> > +        }
> >  
> > -    return 0;
> > +
> 
> 
> > +fail:
> > +    for (ch = 0; ch <= s->max_channel; ch++) {
> > +        if (recompute_sho & (1<<ch)) {
> > +            ChannelParams *cp = &s->channel_params[ch];
> > +
> > +            if (cp->codebook > 0 && cp->huff_lsbs < s->quant_step_size[ch]) {
> > +                if (ret >= 0) {
> > +                    av_log(m->avctx, AV_LOG_ERROR, "quant_step_size larger than huff_lsbs\n");
> > +                    ret = AVERROR_INVALIDDATA;
> > +                }
> > +                s->quant_step_size[ch] = 0;
> > +            }
> > +
> > +            cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
> > +        }
> > +    }
> > +    return ret;
> 
> What's all this stuff for?

As described in the commit message it
"Check quant_step_size against huff_lsbs"

both these are updated independant and conditionally, so the loop
checking them is seperate after both which is ugly.
If you have an idea how to do this cleaner ...

as it is in git the case checked for results in negative shift
exponents

[...]
Michael Niedermayer June 3, 2017, 9:30 p.m. UTC | #3 
On Mon, May 22, 2017 at 05:36:10PM +0200, Michael Niedermayer wrote:
> On Sun, May 21, 2017 at 01:42:18PM +0200, wm4 wrote:
> > On Sat, 20 May 2017 23:01:04 +0200
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > 
> > > This reorders the operations so as to avoid computations with the above arguments
> > > before they have been initialized.
> > > Fixes part of 1708/clusterfuzz-testcase-minimized-5035111957397504
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/mlpdec.c | 34 +++++++++++++++++++++++++---------
> > >  1 file changed, 25 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/libavcodec/mlpdec.c b/libavcodec/mlpdec.c
> > > index c0a23c5f0d..11be380d27 100644
> > > --- a/libavcodec/mlpdec.c
> > > +++ b/libavcodec/mlpdec.c
> > > @@ -825,8 +825,6 @@ static int read_channel_params(MLPDecodeContext *m, unsigned int substr,
> > >          return AVERROR_INVALIDDATA;
> > >      }
> > >  
> > > -    cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
> > > -
> > >      return 0;
> > >  }
> > >  
> > > @@ -838,7 +836,8 @@ static int read_decoding_params(MLPDecodeContext *m, GetBitContext *gbp,
> > >  {
> > >      SubStream *s = &m->substream[substr];
> > >      unsigned int ch;
> > > -    int ret;
> > > +    int ret = 0;
> > > +    unsigned recompute_sho = 0;
> > >  
> > >      if (s->param_presence_flags & PARAM_PRESENCE)
> > >          if (get_bits1(gbp))
> > > @@ -878,19 +877,36 @@ static int read_decoding_params(MLPDecodeContext *m, GetBitContext *gbp,
> > >      if (s->param_presence_flags & PARAM_QUANTSTEP)
> > >          if (get_bits1(gbp))
> > >              for (ch = 0; ch <= s->max_channel; ch++) {
> > > -                ChannelParams *cp = &s->channel_params[ch];
> > > -
> > >                  s->quant_step_size[ch] = get_bits(gbp, 4);
> > >  
> > > -                cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
> > > +                recompute_sho |= 1<<ch;
> > >              }
> > >  
> > >      for (ch = s->min_channel; ch <= s->max_channel; ch++)
> > > -        if (get_bits1(gbp))
> > > +        if (get_bits1(gbp)) {
> > > +            recompute_sho |= 1<<ch;
> > >              if ((ret = read_channel_params(m, substr, gbp, ch)) < 0)
> > > -                return ret;
> > > +                goto fail;
> > > +        }
> > >  
> > > -    return 0;
> > > +
> > 
> > 
> > > +fail:
> > > +    for (ch = 0; ch <= s->max_channel; ch++) {
> > > +        if (recompute_sho & (1<<ch)) {
> > > +            ChannelParams *cp = &s->channel_params[ch];
> > > +
> > > +            if (cp->codebook > 0 && cp->huff_lsbs < s->quant_step_size[ch]) {
> > > +                if (ret >= 0) {
> > > +                    av_log(m->avctx, AV_LOG_ERROR, "quant_step_size larger than huff_lsbs\n");
> > > +                    ret = AVERROR_INVALIDDATA;
> > > +                }
> > > +                s->quant_step_size[ch] = 0;
> > > +            }
> > > +
> > > +            cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
> > > +        }
> > > +    }
> > > +    return ret;
> > 
> > What's all this stuff for?
> 
> As described in the commit message it
> "Check quant_step_size against huff_lsbs"
> 
> both these are updated independant and conditionally, so the loop
> checking them is seperate after both which is ugly.
> If you have an idea how to do this cleaner ...
> 
> as it is in git the case checked for results in negative shift
> exponents

applied

thx

[...]
diff mbox

Patch

diff --git a/libavcodec/mlpdec.c b/libavcodec/mlpdec.c
index c0a23c5f0d..11be380d27 100644
--- a/libavcodec/mlpdec.c
+++ b/libavcodec/mlpdec.c
@@ -825,8 +825,6 @@  static int read_channel_params(MLPDecodeContext *m, unsigned int substr,
         return AVERROR_INVALIDDATA;
     }
 
-    cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
-
     return 0;
 }
 
@@ -838,7 +836,8 @@  static int read_decoding_params(MLPDecodeContext *m, GetBitContext *gbp,
 {
     SubStream *s = &m->substream[substr];
     unsigned int ch;
-    int ret;
+    int ret = 0;
+    unsigned recompute_sho = 0;
 
     if (s->param_presence_flags & PARAM_PRESENCE)
         if (get_bits1(gbp))
@@ -878,19 +877,36 @@  static int read_decoding_params(MLPDecodeContext *m, GetBitContext *gbp,
     if (s->param_presence_flags & PARAM_QUANTSTEP)
         if (get_bits1(gbp))
             for (ch = 0; ch <= s->max_channel; ch++) {
-                ChannelParams *cp = &s->channel_params[ch];
-
                 s->quant_step_size[ch] = get_bits(gbp, 4);
 
-                cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
+                recompute_sho |= 1<<ch;
             }
 
     for (ch = s->min_channel; ch <= s->max_channel; ch++)
-        if (get_bits1(gbp))
+        if (get_bits1(gbp)) {
+            recompute_sho |= 1<<ch;
             if ((ret = read_channel_params(m, substr, gbp, ch)) < 0)
-                return ret;
+                goto fail;
+        }
 
-    return 0;
+
+fail:
+    for (ch = 0; ch <= s->max_channel; ch++) {
+        if (recompute_sho & (1<<ch)) {
+            ChannelParams *cp = &s->channel_params[ch];
+
+            if (cp->codebook > 0 && cp->huff_lsbs < s->quant_step_size[ch]) {
+                if (ret >= 0) {
+                    av_log(m->avctx, AV_LOG_ERROR, "quant_step_size larger than huff_lsbs\n");
+                    ret = AVERROR_INVALIDDATA;
+                }
+                s->quant_step_size[ch] = 0;
+            }
+
+            cp->sign_huff_offset = calculate_sign_huff(m, substr, ch);
+        }
+    }
+    return ret;
 }
 
 #define MSB_MASK(bits)  (-1u << (bits))