diff mbox series

[FFmpeg-devel,v2] avformat/oggdec: Add page CRC verification

Message ID 20200428110546.4295-1-mattias.wadman@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avformat/oggdec: Add page CRC verification
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Mattias Wadman April 28, 2020, 11:05 a.m. UTC
Fixes seek issue with ogg files that have segment data that happens to be
encoded as a "OggS" page syncword. Very unlikely but seems to happen.

Have been observed to happen with ffmpeg+libvorbis and oggenc encoding
to 441khz stereo at 160kbps.
---
 libavformat/oggdec.c | 136 +++++++++++++++++++++++++++++--------------
 1 file changed, 92 insertions(+), 44 deletions(-)

Comments

Mattias Wadman April 28, 2020, 11:15 a.m. UTC | #1
On Tue, Apr 28, 2020 at 1:06 PM Mattias Wadman <mattias.wadman@gmail.com> wrote:
>
> Fixes seek issue with ogg files that have segment data that happens to be
> encoded as a "OggS" page syncword. Very unlikely but seems to happen.
>
> Have been observed to happen with ffmpeg+libvorbis and oggenc encoding
> to 441khz stereo at 160kbps.
> ---
>  libavformat/oggdec.c | 136 +++++++++++++++++++++++++++++--------------
>  1 file changed, 92 insertions(+), 44 deletions(-)
>
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index 95190589ab..0213f1fa12 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -31,6 +31,8 @@
>  #include <stdio.h>
>  #include "libavutil/avassert.h"
>  #include "libavutil/intreadwrite.h"
> +#include "libavutil/crc.h"
> +#include "libavcodec/bytestream.h"
>  #include "oggdec.h"
>  #include "avformat.h"
>  #include "internal.h"
> @@ -205,32 +207,30 @@ static const struct ogg_codec *ogg_find_codec(uint8_t *buf, int size)
>   * situation where a new audio stream spawn (identified with a new serial) and
>   * must replace the previous one (track switch).
>   */
> -static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, int nsegs)
> +static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, uint8_t *segmentsdata, int size)
>  {
>      struct ogg *ogg = s->priv_data;
>      struct ogg_stream *os;
>      const struct ogg_codec *codec;
>      int i = 0;
> +    int magiclen = 8;
>
> -    if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
> -        uint8_t magic[8];
> -        int64_t pos = avio_tell(s->pb);
> -        avio_skip(s->pb, nsegs);
> -        if (avio_read(s->pb, magic, sizeof(magic)) != sizeof(magic))
> -            return AVERROR_INVALIDDATA;
> -        avio_seek(s->pb, pos, SEEK_SET);
> -        codec = ogg_find_codec(magic, sizeof(magic));
> -        if (!codec) {
> -            av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
> -            return AVERROR_INVALIDDATA;
> -        }
> -        for (i = 0; i < ogg->nstreams; i++) {
> -            if (ogg->streams[i].codec == codec)
> -                break;
> -        }
> -        if (i >= ogg->nstreams)
> -            return ogg_new_stream(s, serial);
> -    } else if (ogg->nstreams != 1) {
> +    if (size < magiclen)
> +        return AVERROR_INVALIDDATA;
> +
> +    codec = ogg_find_codec(segmentsdata, magiclen);
> +    if (!codec) {
> +        av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +    for (i = 0; i < ogg->nstreams; i++) {
> +        if (ogg->streams[i].codec == codec)
> +            break;
> +    }
> +    if (i >= ogg->nstreams)
> +        return ogg_new_stream(s, serial);
> +
> +    if (ogg->nstreams != 1) {
>          avpriv_report_missing_feature(s, "Changing stream parameters in multistream ogg");
>          return AVERROR_PATCHWELCOME;
>      }

Not sure what the correct behaviour should be here now. Currently this
check is only done if
the io context is not seekable. But now ogg_replace_stream does need
to seek as it has all data.

> @@ -339,14 +339,24 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
>      AVIOContext *bc = s->pb;
>      struct ogg *ogg = s->priv_data;
>      struct ogg_stream *os;
> -    int ret, i = 0;
> -    int flags, nsegs;
> +    int ret, i;
> +    int version, flags, nsegs;
>      uint64_t gp;
>      uint32_t serial;
> +    uint32_t crc;
>      int size, idx;
>      uint8_t sync[4];
> -    int sp = 0;
> -
> +    uint8_t header[23];
> +    GetByteContext headergb;
> +    uint8_t segments[255];
> +    uint8_t *segmentsdata;
> +    int sp;
> +    const AVCRC *crc_table;
> +    uint32_t ccrc;
> +
> +again:
> +    sp = 0;
> +    i = 0;
>      ret = avio_read(bc, sync, 4);
>      if (ret < 4)
>          return ret < 0 ? ret : AVERROR_EOF;
> @@ -378,47 +388,87 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
>          return AVERROR_INVALIDDATA;
>      }
>
> -    if (avio_r8(bc) != 0) {      /* version */
> -        av_log (s, AV_LOG_ERROR, "ogg page, unsupported version\n");
> +    ret = avio_read(bc, header, sizeof(header));
> +    if (ret < sizeof(header))
> +        return AVERROR_INVALIDDATA;
> +    nsegs = header[22];
> +    ret = avio_read(bc, segments, nsegs);
> +    if (ret < nsegs)
> +        return AVERROR_INVALIDDATA;
> +    size = 0;
> +    for (i = 0; i < nsegs; i++)
> +        size += segments[i];
> +
> +    bytestream2_init(&headergb, header, sizeof(header));
> +    version = bytestream2_get_byte(&headergb);
> +    flags   = bytestream2_get_byte(&headergb);
> +    gp      = bytestream2_get_le64(&headergb);
> +    serial  = bytestream2_get_le32(&headergb);
> +    bytestream2_skip(&headergb, 4); // seq le32
> +    crc     = bytestream2_get_le32(&headergb);
> +
> +    segmentsdata = av_malloc(size);
> +    if (!segmentsdata)
> +        return AVERROR(ENOMEM);
> +    ret = avio_read(bc, segmentsdata, size);
> +    if (ret < size) {
> +        av_freep(&segmentsdata);

Lots if av_freep(&segmentsdata); now. Maybe should refactor to use
RETURN_ERROR macro?

>          return AVERROR_INVALIDDATA;
>      }
>
> -    flags  = avio_r8(bc);
> -    gp     = avio_rl64(bc);
> -    serial = avio_rl32(bc);
> -    avio_skip(bc, 8); /* seq, crc */
> -    nsegs  = avio_r8(bc);
> +    // Reset CRC in header to zero and calculate for whole page
> +    crc_table = av_crc_get_table(AV_CRC_32_IEEE);
> +    memset(&header[18], 0, 4);
> +    ccrc = 0;
> +    ccrc = av_crc(crc_table, ccrc, "OggS", 4);
> +    ccrc = av_crc(crc_table, ccrc, header, sizeof(header));
> +    ccrc = av_crc(crc_table, ccrc, segments, nsegs);
> +    ccrc = av_crc(crc_table, ccrc, segmentsdata, size);
> +    // Default AV_CRC_32_IEEE table is BE
> +    ccrc = av_bswap32(ccrc);
> +
> +    if (ccrc != crc) {
> +        av_log(s, AV_LOG_TRACE, "ogg page, invalid checksum %x != %x\n", ccrc, crc);
> +        if (s->error_recognition & AV_EF_CRCCHECK) {
> +            av_freep(&segmentsdata);
> +            // TODO: smarter use of read data?
> +            goto again;
> +        }
> +    }
>
> -    if (avio_feof(bc))
> -        return AVERROR_EOF;
> +    if (version != 0) {
> +        av_log(s, AV_LOG_ERROR, "ogg page, unsupported version %d\n", version);
> +        av_freep(&segmentsdata);
> +        return AVERROR_INVALIDDATA;
> +    }
>
>      idx = ogg_find_stream(ogg, serial);
>      if (idx < 0) {
>          if (data_packets_seen(ogg))
> -            idx = ogg_replace_stream(s, serial, nsegs);
> +            idx = ogg_replace_stream(s, serial, segmentsdata, size);
>          else
>              idx = ogg_new_stream(s, serial);
>
>          if (idx < 0) {
>              av_log(s, AV_LOG_ERROR, "failed to create or replace stream\n");
> +            av_freep(&segmentsdata);
>              return idx;
>          }
>      }
>
>      os = ogg->streams + idx;
>      ogg->page_pos =
> -    os->page_pos = avio_tell(bc) - 27;
> +    os->page_pos = avio_tell(bc) - 27 - size - nsegs;;

Thinking maybe should store page pos earlier and skip all the
arithmetic here? .. now i see a double ";" hmm

>
>      if (os->psize > 0) {
>          ret = ogg_new_buf(ogg, idx);
> -        if (ret < 0)
> +        if (ret < 0) {
> +            av_freep(&segmentsdata);
>              return ret;
> +        }
>      }
>
> -    ret = avio_read(bc, os->segments, nsegs);
> -    if (ret < nsegs)
> -        return ret < 0 ? ret : AVERROR_EOF;
> -
> +    memcpy(os->segments, segments, nsegs);
>      os->nsegs = nsegs;
>      os->segp  = 0;
>
> @@ -456,10 +506,8 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
>          os->buf = nb;
>      }
>
> -    ret = avio_read(bc, os->buf + os->bufpos, size);
> -    if (ret < size)
> -        return ret < 0 ? ret : AVERROR_EOF;
> -
> +    memcpy(os->buf + os->bufpos, segmentsdata, size);
> +    av_freep(&segmentsdata);
>      os->bufpos += size;
>      os->granule = gp;
>      os->flags   = flags;
> --
> 2.18.0
>
diff mbox series

Patch

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 95190589ab..0213f1fa12 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -31,6 +31,8 @@ 
 #include <stdio.h>
 #include "libavutil/avassert.h"
 #include "libavutil/intreadwrite.h"
+#include "libavutil/crc.h"
+#include "libavcodec/bytestream.h"
 #include "oggdec.h"
 #include "avformat.h"
 #include "internal.h"
@@ -205,32 +207,30 @@  static const struct ogg_codec *ogg_find_codec(uint8_t *buf, int size)
  * situation where a new audio stream spawn (identified with a new serial) and
  * must replace the previous one (track switch).
  */
-static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, int nsegs)
+static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, uint8_t *segmentsdata, int size)
 {
     struct ogg *ogg = s->priv_data;
     struct ogg_stream *os;
     const struct ogg_codec *codec;
     int i = 0;
+    int magiclen = 8;
 
-    if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
-        uint8_t magic[8];
-        int64_t pos = avio_tell(s->pb);
-        avio_skip(s->pb, nsegs);
-        if (avio_read(s->pb, magic, sizeof(magic)) != sizeof(magic))
-            return AVERROR_INVALIDDATA;
-        avio_seek(s->pb, pos, SEEK_SET);
-        codec = ogg_find_codec(magic, sizeof(magic));
-        if (!codec) {
-            av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
-            return AVERROR_INVALIDDATA;
-        }
-        for (i = 0; i < ogg->nstreams; i++) {
-            if (ogg->streams[i].codec == codec)
-                break;
-        }
-        if (i >= ogg->nstreams)
-            return ogg_new_stream(s, serial);
-    } else if (ogg->nstreams != 1) {
+    if (size < magiclen)
+        return AVERROR_INVALIDDATA;
+
+    codec = ogg_find_codec(segmentsdata, magiclen);
+    if (!codec) {
+        av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
+        return AVERROR_INVALIDDATA;
+    }
+    for (i = 0; i < ogg->nstreams; i++) {
+        if (ogg->streams[i].codec == codec)
+            break;
+    }
+    if (i >= ogg->nstreams)
+        return ogg_new_stream(s, serial);
+
+    if (ogg->nstreams != 1) {
         avpriv_report_missing_feature(s, "Changing stream parameters in multistream ogg");
         return AVERROR_PATCHWELCOME;
     }
@@ -339,14 +339,24 @@  static int ogg_read_page(AVFormatContext *s, int *sid)
     AVIOContext *bc = s->pb;
     struct ogg *ogg = s->priv_data;
     struct ogg_stream *os;
-    int ret, i = 0;
-    int flags, nsegs;
+    int ret, i;
+    int version, flags, nsegs;
     uint64_t gp;
     uint32_t serial;
+    uint32_t crc;
     int size, idx;
     uint8_t sync[4];
-    int sp = 0;
-
+    uint8_t header[23];
+    GetByteContext headergb;
+    uint8_t segments[255];
+    uint8_t *segmentsdata;
+    int sp;
+    const AVCRC *crc_table;
+    uint32_t ccrc;
+
+again:
+    sp = 0;
+    i = 0;
     ret = avio_read(bc, sync, 4);
     if (ret < 4)
         return ret < 0 ? ret : AVERROR_EOF;
@@ -378,47 +388,87 @@  static int ogg_read_page(AVFormatContext *s, int *sid)
         return AVERROR_INVALIDDATA;
     }
 
-    if (avio_r8(bc) != 0) {      /* version */
-        av_log (s, AV_LOG_ERROR, "ogg page, unsupported version\n");
+    ret = avio_read(bc, header, sizeof(header));
+    if (ret < sizeof(header))
+        return AVERROR_INVALIDDATA;
+    nsegs = header[22];
+    ret = avio_read(bc, segments, nsegs);
+    if (ret < nsegs)
+        return AVERROR_INVALIDDATA;
+    size = 0;
+    for (i = 0; i < nsegs; i++)
+        size += segments[i];
+
+    bytestream2_init(&headergb, header, sizeof(header));
+    version = bytestream2_get_byte(&headergb);
+    flags   = bytestream2_get_byte(&headergb);
+    gp      = bytestream2_get_le64(&headergb);
+    serial  = bytestream2_get_le32(&headergb);
+    bytestream2_skip(&headergb, 4); // seq le32
+    crc     = bytestream2_get_le32(&headergb);
+
+    segmentsdata = av_malloc(size);
+    if (!segmentsdata)
+        return AVERROR(ENOMEM);
+    ret = avio_read(bc, segmentsdata, size);
+    if (ret < size) {
+        av_freep(&segmentsdata);
         return AVERROR_INVALIDDATA;
     }
 
-    flags  = avio_r8(bc);
-    gp     = avio_rl64(bc);
-    serial = avio_rl32(bc);
-    avio_skip(bc, 8); /* seq, crc */
-    nsegs  = avio_r8(bc);
+    // Reset CRC in header to zero and calculate for whole page
+    crc_table = av_crc_get_table(AV_CRC_32_IEEE);
+    memset(&header[18], 0, 4);
+    ccrc = 0;
+    ccrc = av_crc(crc_table, ccrc, "OggS", 4);
+    ccrc = av_crc(crc_table, ccrc, header, sizeof(header));
+    ccrc = av_crc(crc_table, ccrc, segments, nsegs);
+    ccrc = av_crc(crc_table, ccrc, segmentsdata, size);
+    // Default AV_CRC_32_IEEE table is BE
+    ccrc = av_bswap32(ccrc);
+
+    if (ccrc != crc) {
+        av_log(s, AV_LOG_TRACE, "ogg page, invalid checksum %x != %x\n", ccrc, crc);
+        if (s->error_recognition & AV_EF_CRCCHECK) {
+            av_freep(&segmentsdata);
+            // TODO: smarter use of read data?
+            goto again;
+        }
+    }
 
-    if (avio_feof(bc))
-        return AVERROR_EOF;
+    if (version != 0) {
+        av_log(s, AV_LOG_ERROR, "ogg page, unsupported version %d\n", version);
+        av_freep(&segmentsdata);
+        return AVERROR_INVALIDDATA;
+    }
 
     idx = ogg_find_stream(ogg, serial);
     if (idx < 0) {
         if (data_packets_seen(ogg))
-            idx = ogg_replace_stream(s, serial, nsegs);
+            idx = ogg_replace_stream(s, serial, segmentsdata, size);
         else
             idx = ogg_new_stream(s, serial);
 
         if (idx < 0) {
             av_log(s, AV_LOG_ERROR, "failed to create or replace stream\n");
+            av_freep(&segmentsdata);
             return idx;
         }
     }
 
     os = ogg->streams + idx;
     ogg->page_pos =
-    os->page_pos = avio_tell(bc) - 27;
+    os->page_pos = avio_tell(bc) - 27 - size - nsegs;;
 
     if (os->psize > 0) {
         ret = ogg_new_buf(ogg, idx);
-        if (ret < 0)
+        if (ret < 0) {
+            av_freep(&segmentsdata);
             return ret;
+        }
     }
 
-    ret = avio_read(bc, os->segments, nsegs);
-    if (ret < nsegs)
-        return ret < 0 ? ret : AVERROR_EOF;
-
+    memcpy(os->segments, segments, nsegs);
     os->nsegs = nsegs;
     os->segp  = 0;
 
@@ -456,10 +506,8 @@  static int ogg_read_page(AVFormatContext *s, int *sid)
         os->buf = nb;
     }
 
-    ret = avio_read(bc, os->buf + os->bufpos, size);
-    if (ret < size)
-        return ret < 0 ? ret : AVERROR_EOF;
-
+    memcpy(os->buf + os->bufpos, segmentsdata, size);
+    av_freep(&segmentsdata);
     os->bufpos += size;
     os->granule = gp;
     os->flags   = flags;