Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
Apr 30, 2020, 10:52 by d...@lynne.ee: > Apr 29, 2020, 16:00 by mich...@niedermayer.cc: > >> On Tue, Apr 28, 2020 at 07:22:08PM +0200, Lynne wrote: >> >>> Apr 28, 2020, 16:07 by d...@lynne.ee: >>> >>> > Apr 28, 2020, 15:59 by mattias.wad...@gmail.com: >>> > >>> >> >>> >> >>> > Well, I consider CRC checking a part of correctly parsing ogg, so I think >>> > its best to leave it on >>> > all the time. >>> > >>> >>> Did a new version of the patches. >>> The first two are identical, save for some minor style tweaks. >>> The last one now ensures the max page size, 65k, is available for seeking >>> and speeds up >>> decoding on new/replacement streams since the offset was incorrect. >>> I think its worth it. >>> >>> I'm also close to figuring out how to make chained Opus work, will send a >>> patch once I do. >>> >> >> This patchset seems to detect a crc error in: >> Your patchset is probably correct in this as there are artifacts before, >> just reporting this as i noticed it. >> >> ./ffmpeg -i ~/tickets/2121/vlc-bug.oga c.wav >> >> sample should be here: >> https://trac.ffmpeg.org/raw-attachment/ticket/2121/vlc-bug.oga >> >> thanks >> > > Thanks, checked the sample, it seems corrupt so patch is working as intended. > I plan to push the patchset tonight, unless someone has comments or reviews. > Pushed. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
Apr 29, 2020, 16:00 by mich...@niedermayer.cc: > On Tue, Apr 28, 2020 at 07:22:08PM +0200, Lynne wrote: > >> Apr 28, 2020, 16:07 by d...@lynne.ee: >> >> > Apr 28, 2020, 15:59 by mattias.wad...@gmail.com: >> > >> >> >> >> >> > Well, I consider CRC checking a part of correctly parsing ogg, so I think >> > its best to leave it on >> > all the time. >> > >> >> Did a new version of the patches. >> The first two are identical, save for some minor style tweaks. >> The last one now ensures the max page size, 65k, is available for seeking >> and speeds up >> decoding on new/replacement streams since the offset was incorrect. >> I think its worth it. >> >> I'm also close to figuring out how to make chained Opus work, will send a >> patch once I do. >> > > This patchset seems to detect a crc error in: > Your patchset is probably correct in this as there are artifacts before, > just reporting this as i noticed it. > > ./ffmpeg -i ~/tickets/2121/vlc-bug.oga c.wav > > sample should be here: > https://trac.ffmpeg.org/raw-attachment/ticket/2121/vlc-bug.oga > > thanks > Thanks, checked the sample, it seems corrupt so patch is working as intended. I plan to push the patchset tonight, unless someone has comments or reviews. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
On Tue, Apr 28, 2020 at 07:22:08PM +0200, Lynne wrote: > Apr 28, 2020, 16:07 by d...@lynne.ee: > > > Apr 28, 2020, 15:59 by mattias.wad...@gmail.com: > > > >> > >> > > Well, I consider CRC checking a part of correctly parsing ogg, so I think > > its best to leave it on > > all the time. > > > > Did a new version of the patches. > The first two are identical, save for some minor style tweaks. > The last one now ensures the max page size, 65k, is available for seeking and > speeds up > decoding on new/replacement streams since the offset was incorrect. > I think its worth it. > > I'm also close to figuring out how to make chained Opus work, will send a > patch once I do. > This patchset seems to detect a crc error in: Your patchset is probably correct in this as there are artifacts before, just reporting this as i noticed it. ./ffmpeg -i ~/tickets/2121/vlc-bug.oga c.wav sample should be here: https://trac.ffmpeg.org/raw-attachment/ticket/2121/vlc-bug.oga thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I am the wisest man alive, for I know one thing, and that is that I know nothing. -- Socrates signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
Apr 28, 2020, 16:07 by d...@lynne.ee:
> Apr 28, 2020, 15:59 by mattias.wad...@gmail.com:
>
>>
>>
> Well, I consider CRC checking a part of correctly parsing ogg, so I think its
> best to leave it on
> all the time.
>
Did a new version of the patches.
The first two are identical, save for some minor style tweaks.
The last one now ensures the max page size, 65k, is available for seeking and
speeds up
decoding on new/replacement streams since the offset was incorrect.
I think its worth it.
I'm also close to figuring out how to make chained Opus work, will send a patch
once I do.
>From 3f567301cc771f6192f3991b40805503ad996ff7 Mon Sep 17 00:00:00 2001
From: Lynne
Date: Tue, 28 Apr 2020 12:41:34 +0100
Subject: [PATCH 1/3] oggdec: eliminate copies and extra buffers
This also makes implementing CRC checking far simpler and more robust.
---
libavformat/oggdec.c | 127 ---
1 file changed, 58 insertions(+), 69 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 95190589ab..7db26840b2 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -205,7 +205,7 @@ 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, int size)
{
struct ogg *ogg = s->priv_data;
struct ogg_stream *os;
@@ -214,11 +214,10 @@ static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, int nsegs)
if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
uint8_t magic[8];
-int64_t pos = avio_tell(s->pb);
-avio_skip(s->pb, nsegs);
+avio_seek(s->pb, -size, SEEK_CUR);
if (avio_read(s->pb, magic, sizeof(magic)) != sizeof(magic))
return AVERROR_INVALIDDATA;
-avio_seek(s->pb, pos, SEEK_SET);
+avio_seek(s->pb, size - sizeof(magic), SEEK_CUR);
codec = ogg_find_codec(magic, sizeof(magic));
if (!codec) {
av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
@@ -303,27 +302,6 @@ static int ogg_new_stream(AVFormatContext *s, uint32_t serial)
return idx;
}
-static int ogg_new_buf(struct ogg *ogg, int idx)
-{
-struct ogg_stream *os = ogg->streams + idx;
-uint8_t *nb = av_malloc(os->bufsize + AV_INPUT_BUFFER_PADDING_SIZE);
-int size = os->bufpos - os->pstart;
-
-if (!nb)
-return AVERROR(ENOMEM);
-
-if (os->buf) {
-memcpy(nb, os->buf + os->pstart, size);
-av_free(os->buf);
-}
-
-os->buf= nb;
-os->bufpos = size;
-os->pstart = 0;
-
-return 0;
-}
-
static int data_packets_seen(const struct ogg *ogg)
{
int i;
@@ -343,8 +321,11 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
int flags, nsegs;
uint64_t gp;
uint32_t serial;
-int size, idx;
+int size = 0, idx;
+int64_t page_pos;
uint8_t sync[4];
+uint8_t segments[255];
+uint8_t *readout_buf;
int sp = 0;
ret = avio_read(bc, sync, 4);
@@ -387,47 +368,73 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
gp = avio_rl64(bc);
serial = avio_rl32(bc);
avio_skip(bc, 8); /* seq, crc */
-nsegs = avio_r8(bc);
+
+nsegs= avio_r8(bc);
+page_pos = avio_tell(bc) - 27;
+
+ret = avio_read(bc, segments, nsegs);
+if (ret < nsegs)
+return ret < 0 ? ret : AVERROR_EOF;
if (avio_feof(bc))
return AVERROR_EOF;
+for (i = 0; i < nsegs; i++)
+size += segments[i];
+
idx = ogg_find_stream(ogg, serial);
+if (idx >= 0) {
+os = ogg->streams + idx;
+
+/* Even if invalid guarantee there's enough memory to read the page */
+if (os->bufsize - os->bufpos < size) {
+uint8_t *nb = av_realloc(os->buf, 2*os->bufsize + AV_INPUT_BUFFER_PADDING_SIZE);
+if (!nb)
+return AVERROR(ENOMEM);
+os->buf = nb;
+os->bufsize *= 2;
+}
+
+readout_buf = os->buf + os->bufpos;
+} else {
+readout_buf = av_malloc(size);
+}
+
+ret = avio_read(bc, readout_buf, size);
+if (ret < size) {
+if (idx < 0)
+av_free(readout_buf);
+return ret < 0 ? ret : AVERROR_EOF;
+}
+
if (idx < 0) {
if (data_packets_seen(ogg))
-idx = ogg_replace_stream(s, serial, nsegs);
+idx = ogg_replace_stream(s, serial, 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_free(readout_buf);
return idx;
}
-}
-os = ogg->streams + idx;
-ogg->page_pos =
-os->page_pos = avio_tell(bc) - 27;
+os = ogg->streams + idx;
-if
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
Apr 28, 2020, 15:59 by mattias.wad...@gmail.com: > On Tue, Apr 28, 2020 at 4:45 PM Lynne wrote: > >> >> Apr 28, 2020, 15:31 by mattias.wad...@gmail.com: >> >> > Nice, test files works fine now >> > >> > Would it make sense to conditionally ignore crc mismatch based on >> > s->error_recognition & AV_EF_CRCCHECK ? >> > >> >> I don't think so. This container specifically relies on CRC matching to >> identify packets >> during a seek. While other containers have more advanced sync mechanisms >> beyond a simple >> syncword and checksum, that's all we have here. >> What's worse, we need to be able to handle concatenated ogg files (chained >> opus, vorbis, etc.), >> which are widely used on internet radios. Those have the extradata needed to >> configure the decoder >> on the first packet. If we skip the CRC and misidentify a packet as a >> header, we'll misconfigure the >> decoder and break decoding until the next actual header arrives, which could >> be many minutes. >> The whole chained ogg mechanism is already fragile enough as it >> unfortunately is. >> > > Sorry yes that make sense. I meant more that AV_EF_CRCCHECK seems to > be set by default so adding a > conditionally check would be if someone for some reason really want to > skip it using -err_detect 0 or so. > Well, I consider CRC checking a part of correctly parsing ogg, so I think its best to leave it on all the time. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
Apr 28, 2020, 15:31 by mattias.wad...@gmail.com: > Nice, test files works fine now > > Would it make sense to conditionally ignore crc mismatch based on > s->error_recognition & AV_EF_CRCCHECK ? > I don't think so. This container specifically relies on CRC matching to identify packets during a seek. While other containers have more advanced sync mechanisms beyond a simple syncword and checksum, that's all we have here. What's worse, we need to be able to handle concatenated ogg files (chained opus, vorbis, etc.), which are widely used on internet radios. Those have the extradata needed to configure the decoder on the first packet. If we skip the CRC and misidentify a packet as a header, we'll misconfigure the decoder and break decoding until the next actual header arrives, which could be many minutes. The whole chained ogg mechanism is already fragile enough as it unfortunately is. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
On Tue, Apr 28, 2020 at 4:45 PM Lynne wrote: > > Apr 28, 2020, 15:31 by mattias.wad...@gmail.com: > > > Nice, test files works fine now > > > > Would it make sense to conditionally ignore crc mismatch based on > > s->error_recognition & AV_EF_CRCCHECK ? > > > > I don't think so. This container specifically relies on CRC matching to > identify packets > during a seek. While other containers have more advanced sync mechanisms > beyond a simple > syncword and checksum, that's all we have here. > What's worse, we need to be able to handle concatenated ogg files (chained > opus, vorbis, etc.), > which are widely used on internet radios. Those have the extradata needed to > configure the decoder > on the first packet. If we skip the CRC and misidentify a packet as a header, > we'll misconfigure the > decoder and break decoding until the next actual header arrives, which could > be many minutes. > The whole chained ogg mechanism is already fragile enough as it unfortunately > is. Sorry yes that make sense. I meant more that AV_EF_CRCCHECK seems to be set by default so adding a conditionally check would be if someone for some reason really want to skip it using -err_detect 0 or so. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
On Tue, Apr 28, 2020 at 4:12 PM Lynne wrote: > > Apr 28, 2020, 14:05 by mattias.wad...@gmail.com: > > > On Tue, Apr 28, 2020 at 1:59 PM Lynne wrote: > > > >> > >> This makes decoding far more robust, since OggS, the ogg magic, > >> can be commonly found randomly in streams, which previously made > >> the demuxer think there's a new stream or a change in such. > >> > >> Patch attached. > >> > > > > Maybe nitpick, could seek back even longer than size on crc mismatch? > > > > Thanks a lot for reviewing the patches. > I was thinking about seeking back further to perhaps the version byte, but > unfortunately, that's > not possible. If ffio_ensure_seekback is called multiple times, the last call > overwrites the previous > calls and we lose the ability to seek before it. > Since the only place at which we know the exact size of the page is when its > signaled, that's the > only point we can ensure we can seek back to. > Before that, we can seek back from the header's end to the version byte. > Unfortunately, that > would mean verifying the header before the checksum, which as you've pointed > out, is bad > for robustness. > Still, this is definitely an improvement, since previously the demuxer didn't > seek back at all. I see, thanks for the explaining. Also better than in my patch where it would skip the whole page. > Anyway, I've implemented checking the version byte after reading the CRC as > you suggested > and have attached the new patch. Nice, test files works fine now Would it make sense to conditionally ignore crc mismatch based on s->error_recognition & AV_EF_CRCCHECK ? > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
Apr 28, 2020, 14:05 by mattias.wad...@gmail.com:
> On Tue, Apr 28, 2020 at 1:59 PM Lynne wrote:
>
>>
>> This makes decoding far more robust, since OggS, the ogg magic,
>> can be commonly found randomly in streams, which previously made
>> the demuxer think there's a new stream or a change in such.
>>
>> Patch attached.
>>
>
> Maybe nitpick, could seek back even longer than size on crc mismatch?
>
Thanks a lot for reviewing the patches.
I was thinking about seeking back further to perhaps the version byte, but
unfortunately, that's
not possible. If ffio_ensure_seekback is called multiple times, the last call
overwrites the previous
calls and we lose the ability to seek before it.
Since the only place at which we know the exact size of the page is when its
signaled, that's the
only point we can ensure we can seek back to.
Before that, we can seek back from the header's end to the version byte.
Unfortunately, that
would mean verifying the header before the checksum, which as you've pointed
out, is bad
for robustness.
Still, this is definitely an improvement, since previously the demuxer didn't
seek back at all.
Anyway, I've implemented checking the version byte after reading the CRC as you
suggested
and have attached the new patch.
>From 989d02631f2b47f125d3d8a3474572fe25ab1251 Mon Sep 17 00:00:00 2001
From: Lynne
Date: Tue, 28 Apr 2020 12:52:11 +0100
Subject: [PATCH 2/3] oggdec: verify page checksum
This makes decoding far more robust, since OggS, the ogg magic,
can be commonly found randomly in streams, which previously made
the demuxer think there's a new stream or a change in such.
---
libavformat/oggdec.c | 46
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 7db26840b2..e0188c7c59 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -31,6 +31,7 @@
#include
#include "libavutil/avassert.h"
#include "libavutil/intreadwrite.h"
+#include "avio_internal.h"
#include "oggdec.h"
#include "avformat.h"
#include "internal.h"
@@ -321,8 +322,9 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
int flags, nsegs;
uint64_t gp;
uint32_t serial;
+uint32_t crc, crc_tmp;
int size = 0, idx;
-int64_t page_pos;
+int64_t version, page_pos;
uint8_t sync[4];
uint8_t segments[255];
uint8_t *readout_buf;
@@ -359,15 +361,19 @@ 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");
-return AVERROR_INVALIDDATA;
-}
+/* 0x4fa9b05f = av_crc(AV_CRC_32_IEEE, 0x0, "OggS", 4) */
+ffio_init_checksum(bc, ff_crc04C11DB7_update, 0x4fa9b05f);
-flags = avio_r8(bc);
-gp = avio_rl64(bc);
-serial = avio_rl32(bc);
-avio_skip(bc, 8); /* seq, crc */
+version = avio_r8(bc);
+flags = avio_r8(bc);
+gp = avio_rl64(bc);
+serial = avio_rl32(bc);
+avio_skip(bc, 4); /* seq */
+
+crc_tmp = ffio_get_checksum(bc);
+crc = avio_rb32(bc);
+crc_tmp = ff_crc04C11DB7_update(crc_tmp, (uint8_t[4]){0}, 4);
+ffio_init_checksum(bc, ff_crc04C11DB7_update, crc_tmp);
nsegs= avio_r8(bc);
page_pos = avio_tell(bc) - 27;
@@ -376,9 +382,6 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
if (ret < nsegs)
return ret < 0 ? ret : AVERROR_EOF;
-if (avio_feof(bc))
-return AVERROR_EOF;
-
for (i = 0; i < nsegs; i++)
size += segments[i];
@@ -407,6 +410,25 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
return ret < 0 ? ret : AVERROR_EOF;
}
+if (crc ^ ffio_get_checksum(bc)) {
+av_log(s, AV_LOG_ERROR, "CRC mismatch!\n");
+if (idx < 0)
+av_free(readout_buf);
+avio_seek(bc, -size, SEEK_CUR);
+return 0;
+}
+
+/* Since we're almost sure its a valid packet, checking the version after
+ * the checksum lets the demuxer be more tolerant */
+if (version) {
+av_log(s, AV_LOG_ERROR, "Invalid Ogg vers!\n");
+if (idx < 0)
+av_free(readout_buf);
+avio_seek(bc, -size, SEEK_CUR);
+return 0;
+}
+
+/* CRC is correct so we can be 99% sure there's an actual change here */
if (idx < 0) {
if (data_packets_seen(ogg))
idx = ogg_replace_stream(s, serial, size);
--
2.26.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
On Tue, Apr 28, 2020 at 1:59 PM Lynne wrote: > > This makes decoding far more robust, since OggS, the ogg magic, > can be commonly found randomly in streams, which previously made > the demuxer think there's a new stream or a change in such. > > Patch attached. Maybe nitpick, could seek back even longer than size on crc mismatch? > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
On Tue, Apr 28, 2020 at 1:59 PM Lynne wrote: > > This makes decoding far more robust, since OggS, the ogg magic, > can be commonly found randomly in streams, which previously made > the demuxer think there's a new stream or a change in such. > > Patch attached. Should check version after verifying checksum? this breaks with my test files were the false syncword has a non-zero byte afterwards. > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
This makes decoding far more robust, since OggS, the ogg magic,
can be commonly found randomly in streams, which previously made
the demuxer think there's a new stream or a change in such.
Patch attached.
>From 61759b6b1ef3ca813eb39ee9ace2342eb121b3b0 Mon Sep 17 00:00:00 2001
From: Lynne
Date: Tue, 28 Apr 2020 12:52:11 +0100
Subject: [PATCH 2/3] oggdec: verify page checksum
This makes decoding far more robust, since OggS, the ogg magic,
can be commonly found randomly in streams, which previously made
the demuxer think there's a new stream or a change in such.
---
libavformat/oggdec.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index f67cf42e82..05cea2528b 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -31,6 +31,7 @@
#include
#include "libavutil/avassert.h"
#include "libavutil/intreadwrite.h"
+#include "avio_internal.h"
#include "oggdec.h"
#include "avformat.h"
#include "internal.h"
@@ -321,6 +322,7 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
int flags, nsegs;
uint64_t gp;
uint32_t serial;
+uint32_t crc, crc_tmp;
int size = 0, idx;
int64_t page_pos;
uint8_t sync[4];
@@ -359,6 +361,9 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
return AVERROR_INVALIDDATA;
}
+/* 0x4fa9b05f = av_crc(AV_CRC_32_IEEE, 0x0, "OggS", 4) */
+ffio_init_checksum(bc, ff_crc04C11DB7_update, 0x4fa9b05f);
+
if (avio_r8(bc) != 0) { /* version */
av_log (s, AV_LOG_ERROR, "ogg page, unsupported version\n");
return AVERROR_INVALIDDATA;
@@ -367,7 +372,12 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
flags = avio_r8(bc);
gp = avio_rl64(bc);
serial = avio_rl32(bc);
-avio_skip(bc, 8); /* seq, crc */
+avio_skip(bc, 4); /* seq */
+
+crc_tmp = ffio_get_checksum(bc);
+crc = avio_rb32(bc);
+crc_tmp = ff_crc04C11DB7_update(crc_tmp, (uint8_t[4]){0}, 4);
+ffio_init_checksum(bc, ff_crc04C11DB7_update, crc_tmp);
nsegs = avio_r8(bc);
page_pos = avio_tell(bc) - 27;
@@ -407,6 +417,15 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
return ret < 0 ? ret : AVERROR_EOF;
}
+if (crc ^ ffio_get_checksum(bc)) {
+av_log(s, AV_LOG_ERROR, "CRC mismatch!\n");
+if (idx < 0)
+av_free(readout_buf);
+avio_seek(bc, -size, SEEK_CUR);
+return 0;
+}
+
+/* CRC is correct so we can be 99% sure there's an actual change here */
if (idx < 0) {
if (data_packets_seen(ogg))
idx = ogg_replace_stream(s, serial, size);
--
2.26.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
