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

[Michael Niedermayer]

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.

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 ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

DNS cache poisoning attacks, popular search engine, Google internet authority
dont be evil, please
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151012/a1b4b3d9/attachment.sig>