[FFmpeg-devel] [PATCH 1/2] avcodec/avpacket: Use av_copy_packet_side_data() in av_packet_copy_props()

[James Almer]

On 5/4/2017 7:51 AM, Michael Niedermayer wrote:
> On Wed, May 03, 2017 at 11:27:53PM -0300, James Almer wrote:
>> On 5/3/2017 11:00 PM, Michael Niedermayer wrote:
>>> On Wed, May 03, 2017 at 03:28:53PM +0200, Hendrik Leppkes wrote:
>>>> On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
>>>> <michael at niedermayer.cc> wrote:
>>>>> On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:
>>>>>> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
>>>>>> <michael at niedermayer.cc> wrote:
>>>>>>> On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
>>>>>>>> On Wed, 3 May 2017 11:29:04 +0200
>>>>>>>> Michael Niedermayer <michael at niedermayer.cc> wrote:
>>>>>>>>
>>>>>>>>> On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
>>>>>>>>>> On Wed,  3 May 2017 05:21:50 +0200
>>>>>>>>>> Michael Niedermayer <michael at niedermayer.cc> wrote:
>>>>>>>>>>
>>>>>>>>>>> Fixes timeout
>>>>>>>>>>> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
>>>>>>>>>>>
>>>>>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
>>>>>>>>>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>>>>>>>>>> ---
>>>>>>>>>>>  libavcodec/avpacket.c | 3 +++
>>>>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>>>> index 4bf830bb8a..ff7ee730a4 100644
>>>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>>>> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>>>>>>>>>      dst->flags                = src->flags;
>>>>>>>>>>>      dst->stream_index         = src->stream_index;
>>>>>>>>>>>
>>>>>>>>>>> +    if (!dst->side_data_elems);
>>>>>>>>>>> +        return av_copy_packet_side_data(dst, src);
>>>>>>>>>>> +
>>>>>>>>>>>      for (i = 0; i < src->side_data_elems; i++) {
>>>>>>>>>>>           enum AVPacketSideDataType type = src->side_data[i].type;
>>>>>>>>>>>           int size          = src->side_data[i].size;
>>>>>>>>>>
>>>>>>>>>> This doesn't look right...
>>>>>>>>>
>>>>>>>>> already fixed the ; locally
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>
>>>>>>>> I didn't see that, I was referring to the fact that you call
>>>>>>>> av_copy_packet_side_data(), and only sometimes (at least by intention).
>>>>>>>> That requires at least an explanation in the commit message.
>>>>>>>
>>>>>>> av_packet_copy_props() would add side data to the destination packet
>>>>>>> it doesnt replace previously existing side data except in case of
>>>>>>> error.
>>>>>>> I dont know if that is intended but i didnt want to change it as that
>>>>>>> would be unrelated to this patch
>>>>>>>
>>>>>>
>>>>>> This behavior seems odd at best, so maybe we should just change that
>>>>>> and make the behavior more logical, and fix your issue at the same
>>>>>> time?
>>>>>
>>>>> That can be done and makes alot of sense after the patch.
>>>>>
>>>>> we need to fix this issue in our releases too
>>>>> a simple bugfix and a seperate behavior change afterwards allows us
>>>>> to simply backport the bugfix from master.
>>>>>
>>>>
>>>> If anything your "bugfix" here is a performance improvement, and I
>>>> don't think that warrants backporting either way.
>>>
>>> Before the patch the sample file takes
>>> 51seconds to decode, after it, it takes 203 milliseconds.
>>> The difference is caused by only 2 calls to the copy code
>>>
>>> blocking a machine for 51 seconds for something that decodes in 203ms
>>> otherwise, is IMO worth backporting a fix for.
>>>
>>> We can add a bound to the number of side data elements if theres
>>> consens about doing that and doing it in releases.
>>> still, no code should call realloc() in a loop when it can do one
>>> (re)allocation call at the top.
>>
>> First we need to figure out if these side data copy functions are meant
>> to append the source packet's side data to the dest's, or if they should
>> replace it. That is, we need to see if these functions are meant to
>> assume the dest packet is empty or not. Because judging by every other
>> field copied, i guess it's implied the dest packet is expected to be empty.
>> It's worth nothing that av_stream_add_side_data() overwrites existing
>> side data if one of the same type already exists, unlike
>> av_packet_add_side_data().
> 
> There are 2 Issues
> 
> A. Is that there is a bug in master and the releases that allows one
>    to craft a input which will get you stuck in a realloc loop a long
>    time
> B. The API is not well documented about what should happen if the
>    destination packet isnt empty on a side data copy
> 
> If we fix A and then fix B, then we can backport A without B.
> 
> If we fix B and then fix A then A will likely depend on B and backporting
> just the fix without the API change could be hard.
> Is there a consensus about backporting a change to this API documentation
> and implementation aka "B" ?
> 
> A can be fixed by the proposed patch, by limiting the side data count
> or by first defining the API more clearly aka B and then fixing it
> along the lines of the clear definition.
> 
> What path do people prefer ?

Unless i'm reading this wrong, av_copy_packet_side_data() can leak on
allocation failure. It does dst->side_data_elems = src->side_data_elems
after everything was allocated and copied instead of doing
dst->side_data_elems++ per side data element successfully allocated and
copied.
Not counting that, your patch doesn't seem to change the current
av_packet_copy_props() behavior, which is important for backporting
purposes, so i guess it should be fine to apply.

After that, we'll need to properly document the API as requiring dst to
be empty (or stating it will be overwritten if populated) and adapt the
functions to do that. It's pretty evident it was never meant to be used
with populated packets.