On Sun, Jun 16, 2019 at 09:11:05PM +0200, Michael Niedermayer wrote: > On Sun, Jun 16, 2019 at 12:20:35PM +0200, Lynne wrote: > > Jun 15, 2019, 11:00 PM by mich...@niedermayer.cc: > > > > > Fixes: global-buffer-overflow > > > Fixes: > > > 15247/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5671602181636096 > > > > > > Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc> > > > --- > > > libavcodec/atrac9dec.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/libavcodec/atrac9dec.c b/libavcodec/atrac9dec.c > > > index 805d46f3b8..5401d6e19e 100644 > > > --- a/libavcodec/atrac9dec.c > > > +++ b/libavcodec/atrac9dec.c > > > @@ -535,9 +535,6 @@ static inline void apply_band_extension(ATRAC9Context > > > *s, ATRAC9BlockData *b, > > > at9_q_unit_to_coeff_idx[g_units[3]], > > > }; > > > > > > - if (!b->has_band_ext || !b->has_band_ext_data) > > > - return; > > > - > > > for (int ch = 0; ch <= stereo; ch++) { > > > ATRAC9ChannelData *c = &b->channel[ch]; > > > > > > @@ -741,7 +738,9 @@ static int atrac9_decode_block(ATRAC9Context *s, > > > GetBitContext *gb, > > > > > > apply_intensity_stereo(s, b, stereo); > > > apply_scalefactors (s, b, stereo); > > > - apply_band_extension (s, b, stereo); > > > + > > > + if (b->has_band_ext && b->has_band_ext_data) > > > + apply_band_extension (s, b, stereo); > > > > > > > False positive as usual, q_unit_cnt can't be anything out of array since > > its looked up from > > at9_tab_band_q_unit_map. > > I'd really appreciate it if you stopped fixing complaint messages from > > automated tools. > > Especially from overflows and fuzzing timeouts. The latter are completely > > useless and > > often make the code look worse and weird, and the former are all useless > > except when > > outside of DSP code (e.g. malloc). And most of our code is DSP. > > Calm down please, ill explain how this is reading out of array > > In fact there seem to be more ways than i realized before that this can read > out of array, so i will post 2 more patches to fix this more completely > > First q_unit_cnt is only set from at9_tab_band_q_unit_map if you are lucky as > the code is conditional on a bit read from the bitstream. > > Second the values in at9_tab_band_q_unit_map start like this 0, 4, 8, 10, 12 > > The code reading out of array is this: > > const int g_units[4] = { /* A, B, C, total units */ > b->q_unit_cnt, > at9_tab_band_ext_group[b->q_unit_cnt - 13][0], > at9_tab_band_ext_group[b->q_unit_cnt - 13][1], > > if q_unit_cnt is less than 13 the index is negative and that reads out of the > array > teh value is not used later in the sample but the program could have crashed > already > > It is very good that we discuss this here though as the fix from the patch > does not appear to be enough. There are more pathes that can lead to this.
this patch here is still needed, so i will apply it in the next days unless someone objects or has a better idea thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The bravest are surely those who have the clearest vision of what is before them, glory and danger alike, and yet notwithstanding go out to meet it. -- Thucydides
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".