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

[Carl Eugen Hoyos]

2017-11-14 23:48 GMT+01:00 Carl Eugen Hoyos <ceffmpeg at gmail.com>:
> 2017-11-13 21:07 GMT+01:00 Thilo Borgmann <thilo.borgmann at mail.de>:
>> Am 13.11.17 um 21:06 schrieb Thilo Borgmann:
>>> Am 13.11.17 um 20:02 schrieb Umair Khan:
>>>> Hi,
>>>>
>>>> On Mon, Nov 13, 2017 at 11:06 PM, Thilo Borgmann <thilo.borgmann at mail.de> wrote:
>>>>> Hi,
>>>>>
>>>>>> On Mon, Nov 13, 2017 at 1:09 AM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>>>>>>> 2017-11-12 20:30 GMT+01:00 Umair Khan <omerjerk at gmail.com>:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Mon, Nov 13, 2017 at 12:45 AM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>>>>>>>>> 2017-11-12 20:05 GMT+01:00 Umair Khan <omerjerk at gmail.com>:
>>>>>>>>>
>>>>>>>>>> The attached patch fixes the address sanitizer issue.
>>>>>>>>>
>>>>>>>>> Breaks compilation here, how did you test?
>>>>>>>>>
>>>>>>>>> libavcodec/alsdec.c: In function ‘decode_var_block_data’:
>>>>>>>>> libavcodec/alsdec.c:938:7: error: expected ‘}’ before ‘else’
>>>>>>>>
>>>>>>>> Sorry for the faulty patch. Here is the fixed one.
>>>>>>>
>>>>>>> The commit message of your patch is:
>>>>>>> libavcodec/als: fix address sanitization error in decoder
>>>>>>>
>>>>>>> Is there an error in current FFmpeg git head that asan
>>>>>>> shows? If not, the commit message makes no sense.
>>>>>>>
>>>>>>> I believe you should send two patches that are meant
>>>>>>> to be committed together, one of them fixing ticket #6297.
>>>>>>
>>>>>> This is the complete patchset.
>>>>>
>>>>> I need some days to find time to test this, earliest during the weekend I fear...
>>>>>
>>>>> What happens for
>>>>> block_length < residual_index < opt_order?
>>>>
>>>> I didn't really understand this case. What's residual_index? Can you
>>>> point to the source exactly?
>>>>
>>>> As far as the case where opt_order is more than block_length, my
>>>> second patch handles that case only. The file which Michael sent was
>>>> having asan issues because of the case when block_length < opt_order.
>>>>
>>>>> Another way of asking would be, where is the second loop from specs page 30 for that case?
>>>>> (ISO/IEC 14496)
>>>>
>>>> The second loop is just converting parcor to lpc coefficients which is
>>>> done here - https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/alsdec.c#L935
>>>>
>>>>> I think what puzzles CE is, that the problematic if() from the other patches is still untouched by your patch. So how could this be a valid solution even if your patch would actually improve the prediction part...
>>>>> And I wonder the same ;)
>>>>
>>>> As said, it is valid to have opt_order greater than block_length.
>>>> However, the decoder loop needs to be checked because we won't predict
>>>> values more than the length of the block i.e., block_length. We use
>>>> last K (prediction order, opt_order) values to predict the original K
>>>> values of the current block.
>>>>
>>>>> Did you run FATE with your patch applied? I assume a big difference in output at the first glance (means FATE aks the conformance files should fail...)
>>>>
>>>> Yes. I did run FATE. It passes perfectly.
>>>>
>>>>> Thanks for driving this forward anyway :)
>>>>
>>>> I think the two patches fix the issues completely. I don't see any
>>>> harm in applying this patchset. :)
>>>
>>> Which second patch exactly do you want to be applied along with this one?
>>
>> For convenience just send a patch that does both the changes you want to be done...

Sorry:

Why do you want to merge two patches that are not necessarily related:
Isn't one the revert that fixes a regression and one the new fix for the
overread.

Carl Eugen