From patchwork Tue Apr 28 11:05:47 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mattias Wadman X-Patchwork-Id: 19312 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id B88D444A6D4 for ; Tue, 28 Apr 2020 14:06:50 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 7CB4268BE42; Tue, 28 Apr 2020 14:06:50 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-lj1-f193.google.com (mail-lj1-f193.google.com [209.85.208.193]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 59B5968BDCB for ; Tue, 28 Apr 2020 14:06:44 +0300 (EEST) Received: by mail-lj1-f193.google.com with SMTP id e25so21013397ljg.5 for ; Tue, 28 Apr 2020 04:06:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=o3iSn2jJNpel0y9vDQGIrkqyId1eff6j7C5bjC60Vc0=; b=i2ORkPhdv+oIVLsCv+D3dOfj8hnt0/g5VJuCgNL7+EqU1poorTcAz90SGf/qNbwvRh clWeYEIhNmjWDYMDcakorB+SVKP2AF6f7+HELlQihcBZ+0Oqf0qU38nNEkTJBHu1CPBo NDLAgqrGy1BYf+oGhE7XBO/dzWbraEJ9jOFc6EQh8ZEC+YwQg//yykNG8UcGhy46v5yW VEbuQs3ey+5lDLVjfDV1/BlHKIo9C5gMbI9aPlhwja251f//eO4idEE8IcUKBVS3Yb6F qoX9vkvLhaI87L8wiK3D1yH2+LZFOVhrvLyMcwkSdwWCZq29h9V/IPUbAC7B9OzkYQlm sb/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=o3iSn2jJNpel0y9vDQGIrkqyId1eff6j7C5bjC60Vc0=; b=Oln8jgPMiRk8ZTCOb+n0fwF4gK9LRW3alEEusS8KyZpdpuQusj5sRdaMhWgkTjgF7+ IamI/MylcgJe2A1J6E/c0yrZmLMUB8c8kGQYlCfNrptODpibD3RzPjwp5I9OT8hfLmbf cqqcGbgaH7J03nr1L44UjpQaBWjA8aDxJNI0BSzGR1EZURHLZLYK8cx0thhmXU2eGNb8 QIjtFKYnrnxft6mHleqCZazAhHcMYXzUXrA51kILx3+4xJonFlYIo9kZlOtLGhs/ArkH iKu8PCOm2GD4XtD33JPWY9vi/VoBp/IWjH3MmKZ51az9iQSxdpyKqm01atqTPgNN3/f1 Yb4g== X-Gm-Message-State: AGi0PuYIymZKRoY9ERvip9JwD57gMj8OMof7jTD/JmlvXh/7UcHJmANJ Z4OVDhcX/cj/PDC+5UaQA/ILEISxC6o= X-Google-Smtp-Source: APiQypID+X5jJvf/CzF8eXm+YgITil7m1ty6zA2yDKPgh4/PMBmL8bP1pHqcbK2XzaL0ZKF5Fg6Xfg== X-Received: by 2002:a2e:9791:: with SMTP id y17mr17552663lji.174.1588072002859; Tue, 28 Apr 2020 04:06:42 -0700 (PDT) Received: from work-2.lan (c-388de253.016-130-73746f46.bbcust.telenor.se. [83.226.141.56]) by smtp.gmail.com with ESMTPSA id a11sm12601141lji.62.2020.04.28.04.06.41 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 28 Apr 2020 04:06:42 -0700 (PDT) From: Mattias Wadman To: ffmpeg-devel@ffmpeg.org Date: Tue, 28 Apr 2020 13:05:47 +0200 Message-Id: <20200428110546.4295-1-mattias.wadman@gmail.com> X-Mailer: git-send-email 2.18.0 Subject: [FFmpeg-devel] [PATCH v2] avformat/oggdec: Add page CRC verification X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Mattias Wadman MIME-Version: 1.0 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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 #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; } @@ -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); 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;; 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;