Message ID | 20170520210104.4943-3-michael@niedermayer.cc |
---|---|
State | Accepted |
Commit | 361e0310d95bf2a0377f168518d1135ae15ca3f8 |
Headers | show |
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))
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 [...]
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 --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))
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(-)