[FFmpeg-devel] libavcodec/als: remove check for predictor order of a block

[Umair Khan]

Hi,

On Sat, Nov 4, 2017 at 3:11 AM, Thilo Borgmann <thilo.borgmann at mail.de> wrote:
> Am 03.11.17 um 21:13 schrieb Paul B Mahol:
>> On 11/3/17, Thilo Borgmann <thilo.borgmann at mail.de> wrote:
>>> Am 02.11.17 um 21:32 schrieb Umair Khan:
>>>> Hi,
>>>>
>>>> On Fri, Oct 20, 2017 at 1:44 AM, Ronald S. Bultje <rsbultje at gmail.com>
>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Thu, Oct 19, 2017 at 4:03 PM, Umair Khan <omerjerk at gmail.com> wrote:
>>>>>
>>>>>> I tried decoding the file in both the cases and I don't see any
>>>>>> address related error in the console while decoding. Following is the
>>>>>> output after I apply the patch :-
>>>>>>
>>>>> [..]
>>>>>
>>>>>> Is there something which I'm missing?
>>>>>>
>>>>>
>>>>> You need to run under valgrind or compile with address sanitizer support:
>>>>> configure --toolchain=gcc-asan or --toolchain=clang-asan, depending on
>>>>> the
>>>>> name of clang on your system.
>>>>
>>>> Thanks for the help. I was finally able to reproduce the error.
>>>>
>>>> I have been trying to debug this heap-buffer-overflow error for some
>>>> days. I have finally found the source of the issue at least.
>>>>
>>>> https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/alsdec.c#L934
>>>>
>>>> raw_samples pointer is overflowing inside that loop. I haven't thought
>>>> of a proper fix for this yet. I'll look at the documentation to
>>>> understand the logic first.
>>>>
>>>> However, in case someone (Thilo?) already has some idea on fixing it,
>>>> that'd be great.
>>>
>>> I don't remember exactly but you will need to figure out what the actual
>>> limit is for opt_order.
>>>
>>> If I could give a closer hint, this bug would have been fixed a long time
>>> ago...
>>>
>>> You could have a look at the reference codec code and look where they limit
>>> that opt_order/buffer size.
>>
>> There are already patches by Michael and me which deals with this bug.
>> Which you do not want to apply and without real proof.
>
> Yes and no. If you search some more in the archives, you'll also find
> http://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/202513.html
>
> The idea of Michael and you that you are referring to, is basically to double a part of buffer size calculation to be "more" failsafe. However, especially if Umair wants to dig in now, we should better solve the root of the issue and find the actual required buffer size instead of guessing, don't we?
>
> We cannot accept the removal of the predictor order check alltogether because of the fuzzed file we have. This is what I vetoed in the past (IIRC) and has been suggested by myself, Umair and I think Paul, too.
> Wrong?
>
> What we could do and I would not veto on, would be to have a bigger buffer like Michael's in
> http://ffmpeg.org/pipermail/ffmpeg-devel/2016-June/195113.html
> However, this should be tested again and should get a comment in the code because that buffer size is not reflected in the spec as far as I can tell.
> Also, this would not actually remove that bug from my own list about ALS. That might be fine with you but not the way I think we should proceed. I'd rather love to see Umair fixing it correctly.

The attached patch fixes the address sanitizer issue. This check is
also present in the spec PDF(the one Thilo sent me) on page 30.

-Umair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libavcodec-als-fix-address-sanitization-error-in-dec.patch
Type: application/octet-stream
Size: 911 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171113/0d2a5dc5/attachment.obj>