[FFmpeg-devel] [PATCH] avcodec/apedec: use int64_t for FFABS

[Ganesh Ajjanagadde]

On Mon, Oct 12, 2015 at 11:52 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Mon, Oct 12, 2015 at 11:43 AM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
>> On Mon, Oct 12, 2015 at 11:22:17AM -0400, Ganesh Ajjanagadde wrote:
>>> On Mon, Oct 12, 2015 at 11:09 AM, Michael Niedermayer
>>> <michael at niedermayer.cc> wrote:
>>> > On Fri, Oct 09, 2015 at 01:48:10PM -0400, Ganesh Ajjanagadde wrote:
>>> >> res, absres are currently int's, which on most platforms is 32 bits.
>>> >> Unfortunately, data is untrusted, and on line 1267 res is manipulated
>>> >> with data. Thus, res can take on INT32_MIN/INT_MIN with crafted data,
>>> >> making FFABS on line 1282 unsafe.
>>> >>
>>> >> Once again, using FFNABS will make it less readable: logic is less
>>> >> clear, diff is bigger, and there is scope for mistakes during the
>>> >> expression rewrites.
>>> >>
>>> >> Tested with FATE.
>>> >>
>>> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>>> >> ---
>>> >>  libavcodec/apedec.c | 4 ++--
>>> >>  1 file changed, 2 insertions(+), 2 deletions(-)
>>> >>
>>> >> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
>>> >> index fcccfbe..e46558e 100644
>>> >> --- a/libavcodec/apedec.c
>>> >> +++ b/libavcodec/apedec.c
>>> >> @@ -1254,8 +1254,8 @@ static void init_filter(APEContext *ctx, APEFilter *f, int16_t *buf, int order)
>>> >>  static void do_apply_filter(APEContext *ctx, int version, APEFilter *f,
>>> >>                              int32_t *data, int count, int order, int fracbits)
>>> >>  {
>>> >> -    int res;
>>> >> -    int absres;
>>> >> +    int64_t res;
>>> >> +    int64_t absres;
>>> >
>>> > this makes the function slower:
>>> > before: 1636364 decicycles in doapply,     128 runs,      0 skips
>>> > after:  1710778 decicycles in doapply,     128 runs,      0 skips
>>> >
>>> > (this is on x86-64, i assume its a bigger difference on 32bit)
>>> >
>>> > tested with:
>>> > ./ffmpeg -i fate-suite//lossless-audio/luckynight-partial.ape -f null -
>>>
>>> Ok, but don't you agree that overflow is possible? int16 (essentially
>>> the width after the scalar product) + int32 may not fit in int32. If
>>> we want a lossless conversion, int64 must be used; there is no other
>>> option AFAIK.
>>
>> the max output supported looks like 24 bits not 32
>>
>> also whatever the official decoder / encoder do is correct
>> i dont know what they do, but for example if the encoder uses a
>> int32 and that overflows then any lossless decoder must replicate that.
>
> Ah, the joys of multimedia.
>
>>
>> so i cant say just from knowing that there is a overflow that it is
>> neccessarily wrong, it would require a testcase or comparission to
>> what the official encoder(/decoder) do
>>
>> also if >32bit is possible then av_clip_int16(res); and
>> res & INT32_MIN look suspect
>>
>> i dont know what is correct, as i didnt check the official code nor
>> have a testcase ...
>
> I think this is a good one for the friendly fuzzers at Google (they
> still seem to be helping us out), Can you contact them and see if they
> can craft data that causes overflow issues? On any sane implementation
> of integer overflow, this should not be a security vulnerability
> capable of exploitation (these integers are not being used to index a
> buffer for instance). Nevertheless, I think it is worthy of
> investigation on their end.

Forgot to mention: please drop the current patch. Please do have a
look at the avformat/mov one though: it should not suffer from
performance regressions.

>
>>
>> [...]
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> DNS cache poisoning attacks, popular search engine, Google internet authority
>> dont be evil, please
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>