[FFmpeg-devel] [PATCH 1/3] avcodec/aacdec_template: Fix undefined integer overflow in apply_tns()

[Ivan Kalvachev]

On 7/11/17, Michael Niedermayer <michael at niedermayer.cc> wrote:
> On Sun, Jul 02, 2017 at 01:33:16PM +0200, Michael Niedermayer wrote:
>> On Sun, Jul 02, 2017 at 01:14:31PM +0200, wm4 wrote:
>> > On Sun,  2 Jul 2017 04:28:54 +0200
>> > Michael Niedermayer <michael at niedermayer.cc> wrote:
>> >
>> > > Fixes: runtime error: signed integer overflow: -2147483648 -
>> > > 1202286525 cannot be represented in type 'int'
>> > > Fixes: 2071/clusterfuzz-testcase-minimized-6036414271586304
>> > >
>> > > Found-by: continuous fuzzing process
>> > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> > > ---
>> > >  libavcodec/aac_defines.h     | 2 ++
>> > >  libavcodec/aacdec_template.c | 5 +++--
>> > >  2 files changed, 5 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/libavcodec/aac_defines.h b/libavcodec/aac_defines.h
>> > > index 3c79a8a4a1..ee4c73a87d 100644
>> > > --- a/libavcodec/aac_defines.h
>> > > +++ b/libavcodec/aac_defines.h
>> > > @@ -35,6 +35,7 @@
>> > >  #define AAC_RENAME(x)       x ## _fixed
>> > >  #define AAC_RENAME_32(x)    x ## _fixed_32
>> > >  typedef int                 INTFLOAT;
>> > > +typedef unsigned            SUINTFLOAT;
>> > >  typedef int64_t             INT64FLOAT;
>> > >  typedef int16_t             SHORTFLOAT;
>> > >  typedef SoftFloat           AAC_FLOAT;
>> > > @@ -83,6 +84,7 @@ typedef int                 AAC_SIGNE;
>> > >  #define AAC_RENAME(x)       x
>> > >  #define AAC_RENAME_32(x)    x
>> > >  typedef float               INTFLOAT;
>> > > +typedef float               SUINTFLOAT;
>> >
>> > Not more of this damn shit.
>>
>> i dont think i understand your comment
>>
>> The code is templated and uses largely the INTFLOAT data type
>> which is either signed int or float depending on if the code is build
>> for the fixed point or floating point decoder
>>
>> to fix the undefined behavior in the fixed point decoder a type which
>> is unsigned int is the obvious choice.
>> Such type must be float in the floating point decoder.
>>
>> This patch adds such type.
>>
>> do you object to fixing the issue ?
>> do you want to suggest a different solution ?
>
> over a week passed, noone replied.
> Is everyone ok with patch 1/3 ?
> does someone object to it ?
> does anyone have a better solution ?
>
> If noone replies, i will apply this patch, i do not want to leave
> undefined behavior in the codebase.

I actually would request a short note explaining the SUINTFLOAT type usage.
Something like:
+typedef unsigned            SUINTFLOAT; // Equivalent to INTFLOAT,
Used as temporal cast to avoid undefined sign overflow operations.

Maybe add such note to all "signed value in unsigned type" typedefs.