| Message ID | CAA0c1bBNm-gicBqJuskLN6==PMujHXkFakUN+gU4DXdrmujDMA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >
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 >
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 >
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