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

[Ganesh Ajjanagadde]

On Mon, Oct 12, 2015 at 11:23 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Fri, Oct 9, 2015 at 1:48 PM, Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> 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;
>>
>>      while (count--) {
>>          /* round fixedpoint scalar product */
>> --
>> 2.6.1
>
>
> If you want to take this approach, can't you just cast the input argument to
> FFABS instead of this?

Can be done, but I am not sure that it is the only bottleneck.
Furthermore, it does not take care of the loss issue. On the other
hand, who knows: maybe there are constraints on the data, that it can
be only from such and such values, or such constraints can be imposed
outside do_apply_filter. It just looked untrusted to me.

>
> You could also make absres unsigned, no?

How does that help? Generally signed/unsigned conversions have some
unforeseen performance penalties.

>
> Ronald