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

[wm4]

On Thu, 4 May 2017 12:51:14 +0200
Michael Niedermayer <michael at niedermayer.cc> 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 ?

If you feel strongly about it (and that it affects users), feel free to
push this patch to the release branches, but I think it should be
solved in master by making the code _less_ tricky, instead of _more_
tricky.

Regarding av_packet_copy_props(), I'd tend towards thinking that it
should merge the side data with the dst packet, which means if side
data of the same type is present in both packet, the side data of this
type is essentially removed from the dst packet and replaced with the
one from the source packet. Could probably be done by doing this inside
of av_packet_new_side_data().

(Unfortunate that all the side data code is idiotically duplicated in
other areas of the libs which use the side data concept.)