From patchwork Wed Mar 27 11:18:39 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Diego Felix de Souza via ffmpeg-devel X-Patchwork-Id: 12476 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 6A9EF448DC9 for ; Wed, 27 Mar 2019 13:21:13 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 537E768A910; Wed, 27 Mar 2019 13:21:13 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id D5E4B68A945 for ; Wed, 27 Mar 2019 13:21:10 +0200 (EET) Received: by mail-wr1-f68.google.com with SMTP id p10so18121918wrq.1 for ; Wed, 27 Mar 2019 04:21:10 -0700 (PDT) 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=eFVuylNI4eSSunzmU9jqaZV7RUaHaObJ+tLKq9Q8aIQ=; b=NqcvzImy4Kflcv/krvt9QCCCeFmddrJ9zK6ZyomywGYrOt6M1lFUMmkJmYurbLTRQ9 r31JxdAD7oC4Khy0G8FJNcYjdF9j6KcU0ZmQCzS23Cq9GVNCqShxDB8SE06RX0MRhx7V 0C6cX1bgPabId1WUngWqezzWPIve97l8iA8zJbzbetS72bK/u8PKOdx33UvM/spM5sWt XZ2V9lQqLPr7kRiYfWcMyI6r2SI71bTiMcBLu+WEwE2VjDwKfYvwZ23iZegn7Uwz1zU2 iS1fh+VPoQJ7ECs/Yhoey5PSn7eQQIy9QoHzkBPR2YG0c8v6Sndt7d9sjM+vJK2BbkM1 8nVQ== X-Gm-Message-State: APjAAAVAXpieaiTxdgpd1aTh0hApsw8qd1RZpeYv5v3041bAwk0kIY5R ODDRh3s2e/83K90SiwnFHjvrcmqhWGw= X-Google-Smtp-Source: APXvYqxYx2np2jXgulfxJAwOrXPSVX5MSaBDiGKQkeupedVYWZJTTXDJmgazlaZClgIC3ffC7HXmhQ== X-Received: by 2002:adf:fecb:: with SMTP id q11mr21900936wrs.252.1553685670184; Wed, 27 Mar 2019 04:21:10 -0700 (PDT) Received: from localhost.localdomain (ipbcc08c44.dynamic.kabel-deutschland.de. [188.192.140.68]) by smtp.googlemail.com with ESMTPSA id h10sm31745448wrs.27.2019.03.27.04.21.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Mar 2019 04:21:09 -0700 (PDT) To: ffmpeg-devel@ffmpeg.org Date: Wed, 27 Mar 2019 12:18:39 +0100 Message-Id: <20190327111852.3784-9-andreas.rheinhardt@googlemail.com> X-Mailer: git-send-email 2.19.2 In-Reply-To: <20190327111852.3784-1-andreas.rheinhardt@googlemail.com> References: <20190308092604.3752-1-andreas.rheinhardt@googlemail.com> <20190327111852.3784-1-andreas.rheinhardt@googlemail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 08/21] avformat/matroskadec: Improve error messages 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: , X-Patchwork-Original-From: Andreas Rheinhardt via ffmpeg-devel From: Diego Felix de Souza via ffmpeg-devel Reply-To: FFmpeg development discussions and patches Cc: Andreas Rheinhardt , robux4@ycbcr.xyz Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" ebml_read_num had a number of flaws: 1. The check for read errors/EOF was totally wrong. E.g. an EBML number beginning with the invalid 0x00 would be considered a read error, although it is just invalid data. 2. The check for read errors/EOF was done just once, after reading the first byte of the EBML number. But errors/EOF can happen inbetween, of course, and this wasn't checked. 3. There was no way to distinguish when EOF should be an error (because the data has to be there) for which an error message should be emitted and when it is not necessarily an error (namely during parsing of EBML IDs). Such a possibility has been added and used. Some useless initializations were also fixed. Signed-off-by: Andreas Rheinhardt --- libavformat/matroskadec.c | 61 ++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index a6617a607b..aa2266384a 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -820,29 +820,19 @@ static int ebml_level_end(MatroskaDemuxContext *matroska) * Returns: number of bytes read, < 0 on error */ static int ebml_read_num(MatroskaDemuxContext *matroska, AVIOContext *pb, - int max_size, uint64_t *number) + int max_size, uint64_t *number, int eof_forbidden) { - int read = 1, n = 1; - uint64_t total = 0; + int read, n = 1; + uint64_t total; + int64_t pos; - /* The first byte tells us the length in bytes - avio_r8() can normally - * return 0, but since that's not a valid first ebmlID byte, we can - * use it safely here to catch EOS. */ - if (!(total = avio_r8(pb))) { - /* we might encounter EOS here */ - if (!avio_feof(pb)) { - int64_t pos = avio_tell(pb); - av_log(matroska->ctx, AV_LOG_ERROR, - "Read error at pos. %"PRIu64" (0x%"PRIx64")\n", - pos, pos); - return pb->error ? pb->error : AVERROR(EIO); - } - return AVERROR_EOF; - } + /* The first byte tells us the length in bytes - except when it is zero. */ + total = avio_r8(pb); + if (avio_feof(pb)) + goto err; /* get the length of the EBML number */ - read = 8 - ff_log2_tab[total]; - if (read > max_size) { + if (!total || (read = 8 - ff_log2_tab[total]) > max_size) { int64_t pos = avio_tell(pb) - 1; av_log(matroska->ctx, AV_LOG_ERROR, "Invalid EBML number size tag 0x%02x at pos %"PRIu64" (0x%"PRIx64")\n", @@ -855,9 +845,27 @@ static int ebml_read_num(MatroskaDemuxContext *matroska, AVIOContext *pb, while (n++ < read) total = (total << 8) | avio_r8(pb); + if (avio_feof(pb)) { + eof_forbidden = 1; + goto err; + } + *number = total; return read; + +err: + pos = avio_tell(pb); + if (pb->error) { + av_log(matroska->ctx, AV_LOG_ERROR, + "Read error at pos. %"PRIu64" (0x%"PRIx64")\n", + pos, pos); + return pb->error; + } + if (eof_forbidden) + av_log(matroska->ctx, AV_LOG_ERROR, "File ended prematurely " + "at pos. %"PRIu64" (0x%"PRIx64")\n", pos, pos); + return AVERROR_EOF; } /** @@ -868,7 +876,7 @@ static int ebml_read_num(MatroskaDemuxContext *matroska, AVIOContext *pb, static int ebml_read_length(MatroskaDemuxContext *matroska, AVIOContext *pb, uint64_t *number) { - int res = ebml_read_num(matroska, pb, 8, number); + int res = ebml_read_num(matroska, pb, 8, number, 1); if (res > 0 && *number + 1 == 1ULL << (7 * res)) *number = EBML_UNKNOWN_LENGTH; return res; @@ -1010,7 +1018,7 @@ static int matroska_ebmlnum_uint(MatroskaDemuxContext *matroska, { AVIOContext pb; ffio_init_context(&pb, data, size, 0, NULL, NULL, NULL, NULL); - return ebml_read_num(matroska, &pb, FFMIN(size, 8), num); + return ebml_read_num(matroska, &pb, FFMIN(size, 8), num, 1); } /* @@ -1057,7 +1065,7 @@ static int ebml_parse(MatroskaDemuxContext *matroska, EbmlSyntax *syntax, { if (!matroska->current_id) { uint64_t id; - int res = ebml_read_num(matroska, matroska->ctx->pb, 4, &id); + int res = ebml_read_num(matroska, matroska->ctx->pb, 4, &id, 0); if (res < 0) { // in live mode, finish parsing if EOF is reached. return (matroska->is_live && matroska->ctx->pb->eof_reached && @@ -3335,10 +3343,9 @@ static int matroska_parse_block(MatroskaDemuxContext *matroska, AVBufferRef *buf uint64_t num; int trust_default_duration = 1; - if ((n = matroska_ebmlnum_uint(matroska, data, size, &num)) < 0) { - av_log(matroska->ctx, AV_LOG_ERROR, "EBML block data error\n"); + if ((n = matroska_ebmlnum_uint(matroska, data, size, &num)) < 0) return n; - } + data += n; size -= n; @@ -3699,7 +3706,7 @@ static int webm_clusters_start_with_keyframe(AVFormatContext *s) AVPacket *pkt; avio_seek(s->pb, cluster_pos, SEEK_SET); // read cluster id and length - read = ebml_read_num(matroska, matroska->ctx->pb, 4, &cluster_id); + read = ebml_read_num(matroska, matroska->ctx->pb, 4, &cluster_id, 1); if (read < 0 || cluster_id != 0xF43B675) // done with all clusters break; read = ebml_read_length(matroska, matroska->ctx->pb, &cluster_length); @@ -3916,7 +3923,7 @@ static int webm_dash_manifest_cues(AVFormatContext *s, int64_t init_range) // Cues element ID + EBML length of the Cues element. cues_end is // inclusive and the above sum is reduced by 1. uint64_t cues_length = 0, cues_id = 0, bytes_read = 0; - bytes_read += ebml_read_num(matroska, matroska->ctx->pb, 4, &cues_id); + bytes_read += ebml_read_num(matroska, matroska->ctx->pb, 4, &cues_id, 1); bytes_read += ebml_read_length(matroska, matroska->ctx->pb, &cues_length); cues_end = cues_start + cues_length + bytes_read - 1; }