| Message ID | 20170701013821.5448-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Jul 01, 2017 at 03:38:21AM +0200, Paul B Mahol wrote: > This also reverts 18f94df8. > > Fixes #5297. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavcodec/alsdec.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) This avoids the crash with the sample, i will leave review to thilo as he knows ALS much better than i do thx [...]
Am 01.07.17 um 14:23 schrieb Michael Niedermayer: > On Sat, Jul 01, 2017 at 03:38:21AM +0200, Paul B Mahol wrote: >> This also reverts 18f94df8. >> >> Fixes #5297. >> >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> libavcodec/alsdec.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) > > This avoids the crash with the sample, i will leave review to thilo > as he knows ALS much better than i do Will test tomorrow, also the other ALS patch. However, I suspect it works for the fuzzed sample only becuase so many values are not actually used thus assuming/defining them equal to 0 works. Will see if I can create a valid sample that actually uses these since specs allow for it. Might take a while to figure out and if this backfires some day because we don't handle these extra samples properly the bug will not be found this easily... -Thilo
Am 01.07.17 um 03:38 schrieb Paul B Mahol: > This also reverts 18f94df8. > > Fixes #5297. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavcodec/alsdec.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > [...] > @@ -2062,7 +2057,7 @@ static av_cold int decode_init(AVCodecContext *avctx) > channel_size = sconf->frame_length + sconf->max_order; > > ctx->prev_raw_samples = av_malloc_array(sconf->max_order, sizeof(*ctx->prev_raw_samples)); > - ctx->raw_buffer = av_mallocz_array(avctx->channels * channel_size, sizeof(*ctx->raw_buffer)); > + ctx->raw_buffer = av_mallocz_array(avctx->channels * channel_size + sconf->max_order, sizeof(*ctx->raw_buffer)); > ctx->raw_samples = av_malloc_array(avctx->channels, sizeof(*ctx->raw_samples)); This looks like guessing only about a needed overhead to the raw buffer. What is needed and actually done, is an overhead of max_order and that is already added to the buffer length three lines above the proposed change. So this is nothing more than guessing an overhead of 2*max_order should be enough but it does not resolve knowingly any cause of the bug. Therefore no, this still does not get applied until the actual reason of the bug is found and properly fixed. I've got less time than I thought today and I could not dive in as deep as I want. I cannot predict if there will be more time for me to do it soon. So Paul, I'd appreciate if you could solve this bug but I will only accept a proper patch and no guesswork. -Thilo p.s. I have the original pred.wav used by the ticket author and for me both rm22rev2 and rm23 of the reference codec fail to decode pred.mp4 properly. Does anyone else have the same problem with the reference codecs?
On 7/2/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote: > Am 01.07.17 um 03:38 schrieb Paul B Mahol: >> This also reverts 18f94df8. >> >> Fixes #5297. >> >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> libavcodec/alsdec.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> [...] >> @@ -2062,7 +2057,7 @@ static av_cold int decode_init(AVCodecContext >> *avctx) >> channel_size = sconf->frame_length + sconf->max_order; >> >> ctx->prev_raw_samples = av_malloc_array(sconf->max_order, >> sizeof(*ctx->prev_raw_samples)); >> - ctx->raw_buffer = av_mallocz_array(avctx->channels * >> channel_size, sizeof(*ctx->raw_buffer)); >> + ctx->raw_buffer = av_mallocz_array(avctx->channels * >> channel_size + sconf->max_order, sizeof(*ctx->raw_buffer)); >> ctx->raw_samples = av_malloc_array(avctx->channels, >> sizeof(*ctx->raw_samples)); > > This looks like guessing only about a needed overhead to the raw buffer. > What is > needed and actually done, is an overhead of max_order and that is already > added > to the buffer length three lines above the proposed change. So this is > nothing > more than guessing an overhead of 2*max_order should be enough but it does > not > resolve knowingly any cause of the bug. > > Therefore no, this still does not get applied until the actual reason of the > bug > is found and properly fixed. I've got less time than I thought today and I > could > not dive in as deep as I want. I cannot predict if there will be more time > for > me to do it soon. So Paul, I'd appreciate if you could solve this bug but I > will > only accept a proper patch and no guesswork. This is not guesswork, The raw buffers needs this enough bytes. See this line: ctx->raw_samples[0] = ctx->raw_buffer + sconf->max_order; > > -Thilo > > p.s. I have the original pred.wav used by the ticket author and for me both > rm22rev2 and rm23 of the reference codec fail to decode pred.mp4 properly. > Does > anyone else have the same problem with the reference codecs? Reference code crashes all the time.
Am 02.07.17 um 16:13 schrieb Paul B Mahol: > On 7/2/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote: >> Am 01.07.17 um 03:38 schrieb Paul B Mahol: >>> This also reverts 18f94df8. >>> >>> Fixes #5297. >>> >>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>> --- >>> libavcodec/alsdec.c | 7 +------ >>> 1 file changed, 1 insertion(+), 6 deletions(-) >>> [...] >>> @@ -2062,7 +2057,7 @@ static av_cold int decode_init(AVCodecContext >>> *avctx) >>> channel_size = sconf->frame_length + sconf->max_order; >>> >>> ctx->prev_raw_samples = av_malloc_array(sconf->max_order, >>> sizeof(*ctx->prev_raw_samples)); >>> - ctx->raw_buffer = av_mallocz_array(avctx->channels * >>> channel_size, sizeof(*ctx->raw_buffer)); >>> + ctx->raw_buffer = av_mallocz_array(avctx->channels * >>> channel_size + sconf->max_order, sizeof(*ctx->raw_buffer)); >>> ctx->raw_samples = av_malloc_array(avctx->channels, >>> sizeof(*ctx->raw_samples)); >> >> This looks like guessing only about a needed overhead to the raw buffer. >> What is >> needed and actually done, is an overhead of max_order and that is already >> added >> to the buffer length three lines above the proposed change. So this is >> nothing >> more than guessing an overhead of 2*max_order should be enough but it does >> not >> resolve knowingly any cause of the bug. >> >> Therefore no, this still does not get applied until the actual reason of the >> bug >> is found and properly fixed. I've got less time than I thought today and I >> could >> not dive in as deep as I want. I cannot predict if there will be more time >> for >> me to do it soon. So Paul, I'd appreciate if you could solve this bug but I >> will >> only accept a proper patch and no guesswork. > > This is not guesswork, The raw buffers needs this enough bytes. > > See this line: > ctx->raw_samples[0] = ctx->raw_buffer + sconf->max_order; No. The first max_order samples in the raw_buffer are used and the additional size needed for them is already accounted for in calculation of channel_size some lines above the allocation. This leads to raw_samples[0][0] is max_order sample values after the beginning of the buffer so that negative indices of raw_samples[0] work properly. -Thilo
diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c index d95e30d..4a8f13d 100644 --- a/libavcodec/alsdec.c +++ b/libavcodec/alsdec.c @@ -705,11 +705,6 @@ static int read_var_block_data(ALSDecContext *ctx, ALSBlockData *bd) } else { *bd->opt_order = sconf->max_order; } - if (*bd->opt_order > bd->block_length) { - *bd->opt_order = bd->block_length; - av_log(avctx, AV_LOG_ERROR, "Predictor order too large.\n"); - return AVERROR_INVALIDDATA; - } opt_order = *bd->opt_order; if (opt_order) { @@ -2062,7 +2057,7 @@ static av_cold int decode_init(AVCodecContext *avctx) channel_size = sconf->frame_length + sconf->max_order; ctx->prev_raw_samples = av_malloc_array(sconf->max_order, sizeof(*ctx->prev_raw_samples)); - ctx->raw_buffer = av_mallocz_array(avctx->channels * channel_size, sizeof(*ctx->raw_buffer)); + ctx->raw_buffer = av_mallocz_array(avctx->channels * channel_size + sconf->max_order, sizeof(*ctx->raw_buffer)); ctx->raw_samples = av_malloc_array(avctx->channels, sizeof(*ctx->raw_samples)); if (sconf->floating) {
This also reverts 18f94df8. Fixes #5297. Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/alsdec.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)