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

[James Almer]

On 5/12/2017 12:55 PM, wm4 wrote:
> 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?

What was what allowed and used? If you're talking about AV_PKT_DATA_NB,
it's obviously not being used anywhere but libavcodec.

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

That's my point. av_packet_add_side_data() currently allows multiple
side data elements of the same type in a packet, and this patch breaks
that behavior by limiting it to a max of AV_PKT_DATA_NB elements.
It should be changed to behave the same way as
av_stream_add_side_data(), allowing only one side data element per type
in a packet.

I'll send a patch for this.