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

[James Almer]

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.