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

Submitted by Aaron Colwell on Jan. 9, 2017, 6:47 p.m.

Details

Message ID CAA0c1bBNm-gicBqJuskLN6==PMujHXkFakUN+gU4DXdrmujDMA@mail.gmail.com
State New
Headers show

Commit Message

Aaron Colwell Jan. 9, 2017, 6:47 p.m.
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

Comments

James Almer Jan. 10, 2017, 1:52 a.m.
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@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?

>  
>      size = avio_rb32(pb);
>      if (size > atom.size)
> -- 2.11.0.390.gc69c2f50cf-goog
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Aaron Colwell Jan. 10, 2017, 2:47 a.m.
On Mon, Jan 9, 2017 at 6:00 PM James Almer <jamrial@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@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.


>
> >
> >      size = avio_rb32(pb);
> >      if (size > atom.size)
> > -- 2.11.0.390.gc69c2f50cf-goog
> >
> >
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Almer Jan. 10, 2017, 3:21 a.m.
On 1/9/2017 11:47 PM, Aaron Colwell wrote:
> On Mon, Jan 9, 2017 at 6:00 PM James Almer <jamrial@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@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@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Aaron Colwell Jan. 10, 2017, 4:56 a.m.
On Mon, Jan 9, 2017 at 7:27 PM James Almer <jamrial@gmail.com> wrote:

> On 1/9/2017 11:47 PM, Aaron Colwell wrote:
> > On Mon, Jan 9, 2017 at 6:00 PM James Almer <jamrial@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@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.
>

True. I was just trying to put in some minimal validation to ensure we
don't permit invalid files. I could use avio_tell() to make sure I consumed
the whole box, but I'm not sure that is heading in the direction you're
hoping for.

FWIW I work on the spec this is implementing so I am bias towards rejecting
non-compliant files.


>
> 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.
>

Yes you are correct. I guess this is just exchanging one form of being
permissive for another. :/  I can see your point about just skipping the
box contents might be preferable if the minimal validation above isn't
desirable.

It looks like there are several other assumptions this code is making about
only getting valid boxes. I think the "size > atom.size" checks in several
places might be wrong and aren't taking into account the bytes that have
been consumed at various points. I'll probably try to address that in a
different patch though.


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

Ok. Thanks. I appreciate you taking a look at this.

Aaron


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

Patch hide | download patch | download mbox

From a20866dfeae07a5427e8255145f7fe19d846187d Mon Sep 17 00:00:00 2001
From: Aaron Colwell <acolwell@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;
 
     size = avio_rb32(pb);
     if (size > atom.size)
-- 
2.11.0.390.gc69c2f50cf-goog