[FFmpeg-devel] [PATCHv3 3/3] mkv: Export bounds and padding from spherical metadata

[James Almer]

On 2/28/2017 3:21 PM, Vittorio Giovara wrote:
> On Tue, Feb 28, 2017 at 1:15 PM, James Almer <jamrial at gmail.com> wrote:
>> On 2/28/2017 2:46 PM, Vittorio Giovara wrote:
>>> On Tue, Feb 28, 2017 at 11:00 AM, James Almer <jamrial at gmail.com> wrote:
>>>> On 2/22/2017 1:21 PM, James Almer wrote:
>>>>> On 2/21/2017 7:35 PM, Vittorio Giovara wrote:
>>>>
>>>> CCing this one as well.
>>>>
>>>>>> ---
>>>>>>  libavformat/matroskadec.c              | 64 ++++++++++++++++++++++++++++++++--
>>>>>>  tests/ref/fate/matroska-spherical-mono |  6 +++-
>>>>>>  2 files changed, 66 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>>>>> index 7223e94..0a02237 100644
>>>>>> --- a/libavformat/matroskadec.c
>>>>>> +++ b/libavformat/matroskadec.c
>>>>>> @@ -1913,16 +1913,65 @@ static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track)
>>>>>>      AVSphericalMapping *spherical;
>>>>>>      enum AVSphericalProjection projection;
>>>>>>      size_t spherical_size;
>>>>>> +    size_t l, t, r, b;
>>>>>> +    size_t padding = 0;
>>>>>>      int ret;
>>>>>> +    GetByteContext gb;
>>>>>> +
>>>>>> +    bytestream2_init(&gb, track->video.projection.private.data,
>>>>>> +                     track->video.projection.private.size);
>>>>>
>>>>> private.size can be zero and private.data NULL. bytestream2_init()
>>>>> will trigger an assert in those cases.
>>>
>>> :( asserts like this are really dampening, it should be on read not on
>>> creation (if at all)
>>
>> True, guess it should return an error value instead.
> 
> For now what should we do? Just check that private.size and
> private.data are non-0 and return with an error otherwise?

Actually, nevermind. The assert check is for size >= 0, so it's just
checking for negative values (probably fuzzing related). Sorry for
the noise.

The code should be ok, as you're using the checked versions of the
read functions.

> 
>>>>>> +    if (bytestream2_get_byte(&gb) != 0) {
>>>>>> +        av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
>>>>>> +        return 0;
>>>>>> +    }
>>>>>> +
>>>>>> +    bytestream2_skip(&gb, 3); // flags
>>>>>>
>>>>>>      switch (track->video.projection.type) {
>>>>>>      case MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR:
>>>>>> -        projection = AV_SPHERICAL_EQUIRECTANGULAR;
>>>>>> +        if (track->video.projection.private.size == 0)
>>>>>> +            projection = AV_SPHERICAL_EQUIRECTANGULAR;
>>>>>> +        else if (track->video.projection.private.size == 20) {
>>>>>> +            t = bytestream2_get_be32(&gb);
>>>>>> +            b = bytestream2_get_be32(&gb);
>>>>>> +            l = bytestream2_get_be32(&gb);
>>>>>> +            r = bytestream2_get_be32(&gb);
>>>>>> +
>>>>>> +            if (b >= UINT_MAX - t || r >= UINT_MAX - l) {
>>>>>> +                av_log(NULL, AV_LOG_ERROR,
>>>>>> +                       "Invalid bounding rectangle coordinates "
>>>>>> +                       "%zu,%zu,%zu,%zu\n", l, t, r, b);
>>>>>
>>>>> Use SIZE_SPECIFIER instead of zu. Msvc doesn't like the latter.
>>>>> Same with the mov implementation.
>>>
>>> Umh? Isn't %zu supported on any msvc version that matters (2015 onwards)?
>>> https://connect.microsoft.com/VisualStudio/feedback/details/2083820/printf-format-specifier-zu-is-supported-but-not-documented
>>
>> We also support 2012 and 2013. SIZE_SPECIFIER becomes zu for gcc and
>> such, but Iu for MSVC.
> 
> old software..............
>