| Message ID | 20200528121234.15270-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/adpcm_data: extend ff_adpcm_ima_cunning_index_table | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Thu, 28 May 2020 14:12:33 +0200 "Michael Niedermayer" <michael@niedermayer.cc> wrote: > > Fixes: overread by 1 > Fixes: > 21880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ADPCM_IMA_CUNNING_fuzzer-5717917221257216.fuzz > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > -const int8_t ff_adpcm_ima_cunning_index_table[8] = { > - -1, -1, -1, -1, 1, 2, 3, 4, > +const int8_t ff_adpcm_ima_cunning_index_table[9] = { > + -1, -1, -1, -1, 1, 2, 3, 4, 5 > }; > The index table should only ever be indexed between [0,7], so this looks like a bug in adpcm_ima_cunning_expand_nibble() instead. Where would one go to see the inputs for this? I'd like to investigate (and test against the original decoder). Zane
On Thu, May 28, 2020 at 02:06:32PM +0000, Zane van Iperen wrote: > On Thu, 28 May 2020 14:12:33 +0200 > "Michael Niedermayer" <michael@niedermayer.cc> wrote: > > > > > Fixes: overread by 1 > > Fixes: > > 21880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ADPCM_IMA_CUNNING_fuzzer-5717917221257216.fuzz > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > -const int8_t ff_adpcm_ima_cunning_index_table[8] = { > > - -1, -1, -1, -1, 1, 2, 3, 4, > > +const int8_t ff_adpcm_ima_cunning_index_table[9] = { > > + -1, -1, -1, -1, 1, 2, 3, 4, 5 > > }; > > > The index table should only ever be indexed between [0,7], so this > looks like a bug in adpcm_ima_cunning_expand_nibble() instead. maybe it should be only 0..7 but abs(-8 .. 7) needs a 8th entrty or a check > > Where would one go to see the inputs for this? I'd like to investigate > (and test against the original decoder). The file is from a fuzzer so is random trash and it needs target_dec_fuzzer.c to be read. It wont be read as "Pro Pinball Series Soundbank" so i think that file will not be useful to test the behavior of the original decoder as i would assume it will not accept the target_dec_fuzzer data but i surely can send it to you privatly if you need it. just say if you want me to send you the file Thanks [...]
On Thu, 28 May 2020 18:44:55 +0200 "Michael Niedermayer" <michael@niedermayer.cc> wrote: > > The index table should only ever be indexed between [0,7], so this > > looks like a bug in adpcm_ima_cunning_expand_nibble() instead. > > maybe it should be only 0..7 but abs(-8 .. 7) needs a 8th entrty or a > check > > > > > > Where would one go to see the inputs for this? I'd like to > > investigate (and test against the original decoder). > > The file is from a fuzzer so is random trash and it needs > target_dec_fuzzer.c to be read. It wont be read as "Pro Pinball > Series Soundbank" so i think that file will not be useful to test the > behavior of the original decoder as i would assume it will not accept > the target_dec_fuzzer data but i surely can send it to you privatly > if you need it. just say if you want me to send you the file > No need, I've found an existing file that has the same problem. Out of all the 453 files I have, only one of them triggers this. tl;dr: Change the 5 to -1. ff_adpcm_ima_cunning_index_table[abs(nibble)] is wrong in the case where nibble == -8. If you take the unsigned nibble, and apply f(): f(x) = 16 - x if x > 8 else x & 0x7 you'll get the same value as abs() applied with the signed nibble, except for this one case (abs(-8) == 8, f(8) == 0). Instead of changing the abs(), a cleaner fix is to simply change the 5 to -1. That should give the correct output. Zane
On Fri, May 29, 2020 at 03:47:03AM +0000, Zane van Iperen wrote: > On Thu, 28 May 2020 18:44:55 +0200 > "Michael Niedermayer" <michael@niedermayer.cc> wrote: > > > > > The index table should only ever be indexed between [0,7], so this > > > looks like a bug in adpcm_ima_cunning_expand_nibble() instead. > > > > maybe it should be only 0..7 but abs(-8 .. 7) needs a 8th entrty or a > > check > > > > > > > > > > Where would one go to see the inputs for this? I'd like to > > > investigate (and test against the original decoder). > > > > The file is from a fuzzer so is random trash and it needs > > target_dec_fuzzer.c to be read. It wont be read as "Pro Pinball > > Series Soundbank" so i think that file will not be useful to test the > > behavior of the original decoder as i would assume it will not accept > > the target_dec_fuzzer data but i surely can send it to you privatly > > if you need it. just say if you want me to send you the file > > > > No need, I've found an existing file that has the same problem. Out of > all the 453 files I have, only one of them triggers this. > > tl;dr: Change the 5 to -1. > > > ff_adpcm_ima_cunning_index_table[abs(nibble)] is wrong in the case > where nibble == -8. > > If you take the unsigned nibble, and apply f(): > f(x) = 16 - x if x > 8 else x & 0x7 > > you'll get the same value as abs() applied with the signed nibble, > except for this one case (abs(-8) == 8, f(8) == 0). > > > Instead of changing the abs(), a cleaner fix is to simply change the 5 > to -1. That should give the correct output. will apply with -1 thanks [...]
diff --git a/libavcodec/adpcm_data.c b/libavcodec/adpcm_data.c index cb9d20948e..6fbde8aece 100644 --- a/libavcodec/adpcm_data.c +++ b/libavcodec/adpcm_data.c @@ -178,8 +178,8 @@ const int16_t ff_adpcm_mtaf_stepsize[32][16] = { -424, -1273, -2121, -2970, -3819, -4668, -5516, -6365, }, }; -const int8_t ff_adpcm_ima_cunning_index_table[8] = { - -1, -1, -1, -1, 1, 2, 3, 4, +const int8_t ff_adpcm_ima_cunning_index_table[9] = { + -1, -1, -1, -1, 1, 2, 3, 4, 5 }; const int16_t ff_adpcm_ima_cunning_step_table[61] = { diff --git a/libavcodec/adpcm_data.h b/libavcodec/adpcm_data.h index fa8a03ee1f..d678bfc71a 100644 --- a/libavcodec/adpcm_data.h +++ b/libavcodec/adpcm_data.h @@ -42,7 +42,7 @@ extern const int16_t ff_adpcm_yamaha_indexscale[]; extern const int8_t ff_adpcm_yamaha_difflookup[]; extern const int16_t ff_adpcm_afc_coeffs[2][16]; extern const int16_t ff_adpcm_mtaf_stepsize[32][16]; -extern const int8_t ff_adpcm_ima_cunning_index_table[8]; +extern const int8_t ff_adpcm_ima_cunning_index_table[9]; extern const int16_t ff_adpcm_ima_cunning_step_table[61]; #endif /* AVCODEC_ADPCM_DATA_H */
Fixes: overread by 1 Fixes: 21880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ADPCM_IMA_CUNNING_fuzzer-5717917221257216.fuzz Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/adpcm_data.c | 4 ++-- libavcodec/adpcm_data.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)