Message ID | 20200427151948.91897-1-mattias.wadman@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel] avformat/oggdec: Add page CRC verification | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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 [...]
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.
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 --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;