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

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(-)

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
>