diff mbox series

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

Message ID 20200427151948.91897-1-mattias.wadman@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] avformat/oggdec: Add page CRC verification | expand

Checks

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

Commit Message

Mattias Wadman April 27, 2020, 3:19 p.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 | 96 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 73 insertions(+), 23 deletions(-)

Comments

Michael Niedermayer April 27, 2020, 8:55 p.m. UTC | #1
On Mon, Apr 27, 2020 at 05:19:49PM +0200, Mattias Wadman 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 | 96 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 73 insertions(+), 23 deletions(-)

breaks chained ogg

./ffmpeg -threads 1 -i ~/tickets/868/chained_streams.ogg -f null -

frame=  954 fps=0.0 q=-0.0 Lsize=N/A time=00:00:31.79 bitrate=N/A speed= 105x
vs
frame=  120 fps=0.0 q=-0.0 Lsize=N/A time=00:00:03.99 bitrate=N/A speed= 289x

stream should be here:
http://v2v.cc/~j/theora_testsuite/chained_streams.ogg

thx

[...]
Lynne April 28, 2020, 12:01 p.m. UTC | #2
Apr 27, 2020, 21:55 by michael@niedermayer.cc:

> On Mon, Apr 27, 2020 at 05:19:49PM +0200, Mattias Wadman 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 | 96 +++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 73 insertions(+), 23 deletions(-)
>>
>
> breaks chained ogg
>
> ./ffmpeg -threads 1 -i ~/tickets/868/chained_streams.ogg -f null -
>
> frame=  954 fps=0.0 q=-0.0 Lsize=N/A time=00:00:31.79 bitrate=N/A speed= 105x
> vs
> frame=  120 fps=0.0 q=-0.0 Lsize=N/A time=00:00:03.99 bitrate=N/A speed= 289x
>
> stream should be here:
> http://v2v.cc/~j/theora_testsuite/chained_streams.ogg
>
> thx
>

I've sent a patchset which amongst other things, implements this cleanly and does not break that file.
Had this for a long time, wanted to finish Opus chaining, but seeing this made me send it to the list.
Mattias Wadman April 28, 2020, 12:23 p.m. UTC | #3
On Tue, Apr 28, 2020 at 2:01 PM Lynne <dev@lynne.ee> wrote:
>
> Apr 27, 2020, 21:55 by michael@niedermayer.cc:
>
> > On Mon, Apr 27, 2020 at 05:19:49PM +0200, Mattias Wadman 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 | 96 +++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 73 insertions(+), 23 deletions(-)
> >>
> >
> > breaks chained ogg
> >
> > ./ffmpeg -threads 1 -i ~/tickets/868/chained_streams.ogg -f null -
> >
> > frame=  954 fps=0.0 q=-0.0 Lsize=N/A time=00:00:31.79 bitrate=N/A speed= 105x
> > vs
> > frame=  120 fps=0.0 q=-0.0 Lsize=N/A time=00:00:03.99 bitrate=N/A speed= 289x
> >
> > stream should be here:
> > http://v2v.cc/~j/theora_testsuite/chained_streams.ogg
> >
> > thx
> >
>
> I've sent a patchset which amongst other things, implements this cleanly and does not break that file.
> Had this for a long time, wanted to finish Opus chaining, but seeing this made me send it to the list.

Aha thanks. I should have asked on the list before starting, but i
learned a lot.

I will review your patches and try with some files that have the
issue. Unfortunately i can't share them because of legal reasons.

> _______________________________________________
> 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/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;