[FFmpeg-devel] [PATCH] avcodec/avcodec: Limit the number of side data elements per packet

[wm4]

On Fri, 12 May 2017 10:59:50 -0300
James Almer <jamrial at gmail.com> wrote:

> On 5/12/2017 9:20 AM, Michael Niedermayer wrote:
> > On Fri, May 12, 2017 at 06:16:38AM +0200, wm4 wrote:  
> >> On Thu, 11 May 2017 23:27:37 +0200
> >> Michael Niedermayer <michael at niedermayer.cc> wrote:
> >>  
> >>> On Thu, May 11, 2017 at 06:54:16PM +0200, wm4 wrote:  
> >>>> On Thu, 11 May 2017 13:01:36 +0200
> >>>> Michael Niedermayer <michael at niedermayer.cc> wrote:
> >>>>     
> >>>>> 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/avcodec.h  | 8 ++++++++
> >>>>>  libavcodec/avpacket.c | 5 ++++-
> >>>>>  2 files changed, 12 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> >>>>> index df6d2bc748..173c083a86 100644
> >>>>> --- a/libavcodec/avcodec.h
> >>>>> +++ b/libavcodec/avcodec.h
> >>>>> @@ -1593,6 +1593,14 @@ enum AVPacketSideDataType {
> >>>>>       * AVContentLightMetadata struct.
> >>>>>       */
> >>>>>      AV_PKT_DATA_CONTENT_LIGHT_LEVEL,
> >>>>> +
> >>>>> +    /**
> >>>>> +     * The number of side data elements (in fact a bit more than it).
> >>>>> +     * This is not part of the public API/ABI in the sense that it may
> >>>>> +     * change when new side data types are added.
> >>>>> +     * This must stay the last enum value.
> >>>>> +     */
> >>>>> +    AV_PKT_DATA_NB,
> >>>>>  };    
> >>>>
> >>>> OK I guess.
> >>>>     
> >>>>>  #define AV_PKT_DATA_QUALITY_FACTOR AV_PKT_DATA_QUALITY_STATS //DEPRECATED
> >>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >>>>> index 369dd78208..200ba99f34 100644
> >>>>> --- a/libavcodec/avpacket.c
> >>>>> +++ b/libavcodec/avpacket.c
> >>>>> @@ -298,7 +298,7 @@ int av_packet_add_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
> >>>>>      AVPacketSideData *tmp;
> >>>>>      int elems = pkt->side_data_elems;
> >>>>>  
> >>>>> -    if ((unsigned)elems + 1 > INT_MAX / sizeof(*pkt->side_data))
> >>>>> +    if ((unsigned)elems + 1 > FFMIN(INT_MAX / sizeof(*pkt->side_data), AV_PKT_DATA_NB))    
> >>>>
> >>>> Does the FFMIN and the old expression on the right side still have any
> >>>> function?    
> >>>
> >>> In practice, no
> >>> In principle, yes, AV_PKT_DATA_NB could be larger than
> >>> INT_MAX / sizeof(*pkt->side_data  
> >>
> >> That seems extremely unlikely. Feel free to extend the comment on the
> >> new enum item to this extend.
> >>  
> >>>
> >>> do you prefer if i remove it ?  
> >>
> >> Yes.  
> > 
> > changed
> > 
> > applied
> > 
> > thx  
> 
> For the record, by limiting the amount of side data elements to
> AV_PKT_DATA_NB we're potentially breaking the current behavior where the
> function allows more than one element of the same side data type in a
> packet.
> What should be done here is make this function behave the same as
> av_stream_add_side_data() and allow only one element per type,
> overwriting the existing one when a new one is provided.
> 
> I don't know for that matter if the above would have been better than
> adding this _NB enum, or at least sufficient, seeing how
> av_stream_add_side_data() also does a "INT_MAX / sizeof(*side_data)"
> check after making sure only one element per type is allowed, and can't
> really use this _NB enum being in a different library.

How was that ever allowed and where is this used?

I doubt anything can properly deal with it, and duplicate side data
elements should be disallowed.