diff mbox series

[FFmpeg-devel] avformat/dhav: Fix incorrect non-DHAV chunk skipping logic

Message ID 20210113123454.27617-1-idan@tokagroup.com
State Accepted
Headers show
Series [FFmpeg-devel] avformat/dhav: Fix incorrect non-DHAV chunk skipping logic | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Idan Freiberg Jan. 13, 2021, 12:34 p.m. UTC 
DAV files may contain a variable length padding in between chunks
filled with 0xff bytes. The current skipping logic is incorrect as it
may skip over DHAV chunks not appearing sequentially in the file.

We now look for the 'DHAV' tag using a byte-by-byte search in order
to handle such situations. Also the dhav->last_good_pos field will
not be updated while skipping unrecognized data.
---
 libavformat/dhav.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

Comments

Idan Freiberg Jan. 18, 2021, 5:41 p.m. UTC | #1 
Can one of the maintainers review the patch please?

בתאריך יום ד׳, 13 בינו׳ 2021 ב-14:35 מאת Idan Freiberg <speidy@gmail.com>:

> DAV files may contain a variable length padding in between chunks
> filled with 0xff bytes. The current skipping logic is incorrect as it
> may skip over DHAV chunks not appearing sequentially in the file.
>
> We now look for the 'DHAV' tag using a byte-by-byte search in order
> to handle such situations. Also the dhav->last_good_pos field will
> not be updated while skipping unrecognized data.
> ---
>  libavformat/dhav.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/libavformat/dhav.c b/libavformat/dhav.c
> index 00e0d8476e..6a6c235e65 100644
> --- a/libavformat/dhav.c
> +++ b/libavformat/dhav.c
> @@ -173,18 +173,9 @@ static int read_chunk(AVFormatContext *s)
>      if (avio_feof(s->pb))
>          return AVERROR_EOF;
>
> -    if (avio_rl32(s->pb) != MKTAG('D','H','A','V') && dhav->last_good_pos
> < INT64_MAX - 0x8000) {
> -        dhav->last_good_pos += 0x8000;
> -        avio_seek(s->pb, dhav->last_good_pos, SEEK_SET);
> -
> -        while (avio_rl32(s->pb) != MKTAG('D','H','A','V')) {
> -            if (avio_feof(s->pb) || dhav->last_good_pos >= INT64_MAX -
> 0x8000)
> -                return AVERROR_EOF;
> -            dhav->last_good_pos += 0x8000;
> -            ret = avio_skip(s->pb, 0x8000 - 4);
> -            if (ret < 0)
> -                return ret;
> -        }
> +    while (avio_r8(s->pb) != 'D' || avio_r8(s->pb) != 'H' ||
> avio_r8(s->pb) != 'A' || avio_r8(s->pb) != 'V') {
> +        if (avio_feof(s->pb))
> +            return AVERROR_EOF;
>      }
>
>      start = avio_tell(s->pb) - 4;
> --
> 2.30.0
>
> --
Idan Freiberg Mobile: +972-52-2925213
Paul B Mahol Jan. 18, 2021, 5:53 p.m. UTC | #2 
Isn't this applied already?

On Mon, Jan 18, 2021 at 6:46 PM Idan Freiberg <speidy@gmail.com> wrote:

> Can one of the maintainers review the patch please?
>
> בתאריך יום ד׳, 13 בינו׳ 2021 ב-14:35 מאת Idan Freiberg <speidy@gmail.com>:
>
> > DAV files may contain a variable length padding in between chunks
> > filled with 0xff bytes. The current skipping logic is incorrect as it
> > may skip over DHAV chunks not appearing sequentially in the file.
> >
> > We now look for the 'DHAV' tag using a byte-by-byte search in order
> > to handle such situations. Also the dhav->last_good_pos field will
> > not be updated while skipping unrecognized data.
> > ---
> >  libavformat/dhav.c | 15 +++------------
> >  1 file changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/libavformat/dhav.c b/libavformat/dhav.c
> > index 00e0d8476e..6a6c235e65 100644
> > --- a/libavformat/dhav.c
> > +++ b/libavformat/dhav.c
> > @@ -173,18 +173,9 @@ static int read_chunk(AVFormatContext *s)
> >      if (avio_feof(s->pb))
> >          return AVERROR_EOF;
> >
> > -    if (avio_rl32(s->pb) != MKTAG('D','H','A','V') &&
> dhav->last_good_pos
> > < INT64_MAX - 0x8000) {
> > -        dhav->last_good_pos += 0x8000;
> > -        avio_seek(s->pb, dhav->last_good_pos, SEEK_SET);
> > -
> > -        while (avio_rl32(s->pb) != MKTAG('D','H','A','V')) {
> > -            if (avio_feof(s->pb) || dhav->last_good_pos >= INT64_MAX -
> > 0x8000)
> > -                return AVERROR_EOF;
> > -            dhav->last_good_pos += 0x8000;
> > -            ret = avio_skip(s->pb, 0x8000 - 4);
> > -            if (ret < 0)
> > -                return ret;
> > -        }
> > +    while (avio_r8(s->pb) != 'D' || avio_r8(s->pb) != 'H' ||
> > avio_r8(s->pb) != 'A' || avio_r8(s->pb) != 'V') {
> > +        if (avio_feof(s->pb))
> > +            return AVERROR_EOF;
> >      }
> >
> >      start = avio_tell(s->pb) - 4;
> > --
> > 2.30.0
> >
> > --
> Idan Freiberg Mobile: +972-52-2925213
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Idan Freiberg Jan. 19, 2021, 9:57 a.m. UTC | #3 
Oh i didn't notice that. Thanks.

On Mon, Jan 18, 2021 at 8:22 PM Paul B Mahol <onemda@gmail.com> wrote:

> Isn't this applied already?
>
> On Mon, Jan 18, 2021 at 6:46 PM Idan Freiberg <speidy@gmail.com> wrote:
>
> > Can one of the maintainers review the patch please?
> >
> > בתאריך יום ד׳, 13 בינו׳ 2021 ב-14:35 מאת Idan Freiberg <speidy@gmail.com
> >:
> >
> > > DAV files may contain a variable length padding in between chunks
> > > filled with 0xff bytes. The current skipping logic is incorrect as it
> > > may skip over DHAV chunks not appearing sequentially in the file.
> > >
> > > We now look for the 'DHAV' tag using a byte-by-byte search in order
> > > to handle such situations. Also the dhav->last_good_pos field will
> > > not be updated while skipping unrecognized data.
> > > ---
> > >  libavformat/dhav.c | 15 +++------------
> > >  1 file changed, 3 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/libavformat/dhav.c b/libavformat/dhav.c
> > > index 00e0d8476e..6a6c235e65 100644
> > > --- a/libavformat/dhav.c
> > > +++ b/libavformat/dhav.c
> > > @@ -173,18 +173,9 @@ static int read_chunk(AVFormatContext *s)
> > >      if (avio_feof(s->pb))
> > >          return AVERROR_EOF;
> > >
> > > -    if (avio_rl32(s->pb) != MKTAG('D','H','A','V') &&
> > dhav->last_good_pos
> > > < INT64_MAX - 0x8000) {
> > > -        dhav->last_good_pos += 0x8000;
> > > -        avio_seek(s->pb, dhav->last_good_pos, SEEK_SET);
> > > -
> > > -        while (avio_rl32(s->pb) != MKTAG('D','H','A','V')) {
> > > -            if (avio_feof(s->pb) || dhav->last_good_pos >= INT64_MAX -
> > > 0x8000)
> > > -                return AVERROR_EOF;
> > > -            dhav->last_good_pos += 0x8000;
> > > -            ret = avio_skip(s->pb, 0x8000 - 4);
> > > -            if (ret < 0)
> > > -                return ret;
> > > -        }
> > > +    while (avio_r8(s->pb) != 'D' || avio_r8(s->pb) != 'H' ||
> > > avio_r8(s->pb) != 'A' || avio_r8(s->pb) != 'V') {
> > > +        if (avio_feof(s->pb))
> > > +            return AVERROR_EOF;
> > >      }
> > >
> > >      start = avio_tell(s->pb) - 4;
> > > --
> > > 2.30.0
> > >
> > > --
> > Idan Freiberg Mobile: +972-52-2925213
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/dhav.c b/libavformat/dhav.c
index 00e0d8476e..6a6c235e65 100644
--- a/libavformat/dhav.c
+++ b/libavformat/dhav.c
@@ -173,18 +173,9 @@  static int read_chunk(AVFormatContext *s)
     if (avio_feof(s->pb))
         return AVERROR_EOF;
 
-    if (avio_rl32(s->pb) != MKTAG('D','H','A','V') && dhav->last_good_pos < INT64_MAX - 0x8000) {
-        dhav->last_good_pos += 0x8000;
-        avio_seek(s->pb, dhav->last_good_pos, SEEK_SET);
-
-        while (avio_rl32(s->pb) != MKTAG('D','H','A','V')) {
-            if (avio_feof(s->pb) || dhav->last_good_pos >= INT64_MAX - 0x8000)
-                return AVERROR_EOF;
-            dhav->last_good_pos += 0x8000;
-            ret = avio_skip(s->pb, 0x8000 - 4);
-            if (ret < 0)
-                return ret;
-        }
+    while (avio_r8(s->pb) != 'D' || avio_r8(s->pb) != 'H' || avio_r8(s->pb) != 'A' || avio_r8(s->pb) != 'V') {
+        if (avio_feof(s->pb))
+            return AVERROR_EOF;
     }
 
     start = avio_tell(s->pb) - 4;