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

[Mattias Wadman]

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 | 96 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 73 insertions(+), 23 deletions(-)

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 95190589ab..22532f0341 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"
@@ -339,14 +341,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,19 +390,59 @@ 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) {
@@ -401,24 +453,24 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
 
         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 +508,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