[FFmpeg-devel] [PATCH] mov: Fix spherical metadata_source field parsing.

[James Almer]

On 1/9/2017 11:47 PM, Aaron Colwell wrote:
> On Mon, Jan 9, 2017 at 6:00 PM James Almer <jamrial at gmail.com> wrote:
> 
>> On 1/9/2017 3:47 PM, Aaron Colwell wrote:
>>> The attached patch fixes MOV spherical metadata parsing when the
>>> metadata_source field is not an empty string.
>>>
>>> The metadata_source field is a null-terminated string, like other ISOBMFF
>>> strings,
>>> not an 8-bit length followed by string characters. This patch fixes the
>>> parsing
>>> code so it skips over the string properly.
>>>
>>> Aaron
>>>
>>>
>>> 0001-mov-Fix-spherical-metadata_source-field-parsing.patch
>>>
>>>
>>> From a20866dfeae07a5427e8255145f7fe19d846187d Mon Sep 17 00:00:00 2001
>>> From: Aaron Colwell <acolwell at google.com>
>>> Date: Mon, 9 Jan 2017 09:58:01 -0800
>>> Subject: [PATCH] mov: Fix spherical metadata_source field parsing.
>>>
>>> The metadata_source field is a null-terminated string like other ISOBMFF
>> strings
>>> not an 8-bit length followed by string characters. This patch fixes the
>> parsing
>>> code so it skips over the string properly.
>>> ---
>>>  libavformat/mov.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index d1b929174d..4399d2ab13 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -4553,6 +4553,7 @@ static int mov_read_sv3d(MOVContext *c,
>> AVIOContext *pb, MOVAtom atom)
>>>      int32_t yaw, pitch, roll;
>>>      uint32_t tag;
>>>      enum AVSphericalProjection projection;
>>> +    int i;
>>>
>>>      if (c->fc->nb_streams < 1)
>>>          return 0;
>>> @@ -4575,7 +4576,11 @@ static int mov_read_sv3d(MOVContext *c,
>> AVIOContext *pb, MOVAtom atom)
>>>          return 0;
>>>      }
>>>      avio_skip(pb, 4); /*  version + flags */
>>> -    avio_skip(pb, avio_r8(pb)); /* metadata_source */
>>> +
>>> +    /* metadata_source */
>>> +    for (i = 0; i < size - 12; ++i)
>>> +        if (!avio_r8(pb))
>>> +            break;
>>
>> Wouldn't just doing avio_skip(pb, size - 12) be enough for this?
>>
> 
> Yes. That would be the smallest change that could possibly work, but would
> be more permissive than what the spec requires. I was trying to fix the bug
> and
> have bad content trigger an error.

But unless we decide to export it as metadata, we don't currently care about
the contents of the box.

The size value is expected to be correct in a sane file. If it isn't and if
we skip said amount of bytes, the code expecting the following box will
promptly fail.
With your code, and assuming I'm reading it right and not missing something,
if a null byte is found before the reported svhd box size is fully consumed
we would in fact be being more permissive by letting the demuxer continue if
the next box effectively starts right after said null byte, regardless of
what the svhd box size reported.

In any case I'm CCing the author of this code to see what he prefers.

>>
>>>
>>>      size = avio_rb32(pb);
>>>      if (size > atom.size)
>>> -- 2.11.0.390.gc69c2f50cf-goog
>>>
>>>
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>