From patchwork Thu Mar 26 00:41:43 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 18401 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 C73FC4482A4 for ; Thu, 26 Mar 2020 02:42:15 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id B1CDE68B515; Thu, 26 Mar 2020 02:42:15 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 8F030689C6C for ; Thu, 26 Mar 2020 02:42:09 +0200 (EET) Received: by mail-wr1-f67.google.com with SMTP id b2so5738754wrj.10 for ; Wed, 25 Mar 2020 17:42:09 -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:in-reply-to:references :mime-version:content-transfer-encoding; bh=wPmw1lzZ0lmoGM3EB8u4VOYVOMh5r+DcvQZU1TnHoZo=; b=BXL4+piR/VZv8CycdoM1MijS9wMqlHk7kMX+l93lC7G6tm7jcGOSB+09yhsiUZyxMv pzINcLASydsxV09k8AmAKJ6BUVHEc7a5JnbikdQUVJ/aIEMQW3T+vy0zJgVgOZi3Aj9y toV3buvRsSpiVpX8JnEFePH39ATSjE/IY7DD6eFS452eGahXTEUpBroVQbaUUa3c9IM8 ytmlf+1tVj1kNnMH5/kA4cOWJtD0+/k2jA8y/EhintIClluRnEN9jzRxRr74FjRfSrNi OQPpF7IsYpGFPZQBtb0/Mi41PvHLCEtjrq8jfU5HJUnC7E7hH3zow1ejB9iIo3gS/WBM Dkdw== 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=wPmw1lzZ0lmoGM3EB8u4VOYVOMh5r+DcvQZU1TnHoZo=; b=L39kZIMbSm/LFJ00qq0GvNbaYP5oQpkdjcksdfYt8dQjRua4evYZuf+XpmhCd6eCYX QTxzdsSbp7DbKWFKKy82e3L6c6VimlylqcoghVZFx3KoYEoL7nW6jSlUtB76xBu4cJdm PiGk4VCWZ9TiKFGrR3sVy1adX25hBpHh5kSrGF2PI6kGz8uSe9lv9MukzhZioZ2wNIFs y/CJ3eG5rVmknES85EBGsUTd2syf0UpDv/iFcvNCGvrw04iV0w0SyUYGU+3HFGjpmrDe ng6Tz2trX941eIIH8Shp6OR33NuoN2qX9BTgn1zZfKazvH1fnKbmANBg4ZxTip+9+0RF DasA== X-Gm-Message-State: ANhLgQ3uEA5NMwuAhL9n3Ouq6deq01VKMHI6r5xpkO9PtMnT55iPtqn6 hq1hLSV6yyOAf8HK7wgxfqv/5CSl X-Google-Smtp-Source: ADFU+vtOgdnrKB8AVnCSmOBFuYEZt9MXZyCepyWag4lAYq4WE+ju1Mmcj6UuimXThjoDMeotxVJqxQ== X-Received: by 2002:adf:e584:: with SMTP id l4mr2181625wrm.388.1585183328484; Wed, 25 Mar 2020 17:42:08 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc1ab57.dynamic.kabel-deutschland.de. [188.193.171.87]) by smtp.gmail.com with ESMTPSA id q72sm1142723wme.31.2020.03.25.17.42.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Mar 2020 17:42:07 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Thu, 26 Mar 2020 01:41:43 +0100 Message-Id: <20200326004144.26758-2-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200326004144.26758-1-andreas.rheinhardt@gmail.com> References: <20200326004144.26758-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 2/3] avformat/matroskadec: Don't discard valid packets 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: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" A Block (meaning both a Block in a BlockGroup as well as a SimpleBlock) must have at least three bytes after the field containing the encoded TrackNumber. So a if there are <= 3 bytes, the Matroska demuxer would skip this block, believing it to be an empty, but valid Block. This might discard valid Blocks, even nonempty ones (namely if the track uses header stripping). And certain definitely spec-incompliant Blocks don't raise errors: Those with two or less bytes left after the encoded TrackNumber and those with three bytes left, but with flags indicating that the Block uses lacing (in which case there has to be further data describing the lacing). Furthermore, zero-sized packets were still possible because only the size of the last entry of a lace was checked. This commit fixes this. All spec-compliant Blocks are now returned to the caller, even those with a size of zero. This is in accordance with the documentation of av_read_frame(): "This function returns what is stored in the file, and does not validate that what is there are valid frames for the decoder". Signed-off-by: Andreas Rheinhardt --- mkclean can produce blocks where the payload has a size of zero before readding the removed header. E.g. it does this if a stream only has one frame in total (although in such a case the overhead of header removal compression is greater than the number of bytes saved in the Blocks); this can in particular happen with forced subtitle tracks with only one frame (which is how I noticed this). I am unsure what to do with size-zero packets; I don't know any Matroska Codec Mapping where this would be allowed. But there is nothing explicitly stating that they are generally illegal, so this commit returns them. The only thing I am certain is that stripping these packets away needs to happen after the content compressions have been reversed. libavformat/matroskadec.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 6425973954..7aea13dda0 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -2991,7 +2991,9 @@ static int matroska_parse_laces(MatroskaDemuxContext *matroska, uint8_t **buf, return 0; } - av_assert0(size > 0); + if (size <= 0) + return AVERROR_INVALIDDATA; + *laces = *data + 1; data += 1; size -= 1; @@ -3017,7 +3019,7 @@ static int matroska_parse_laces(MatroskaDemuxContext *matroska, uint8_t **buf, break; } } - if (size <= total) { + if (size < total) { return AVERROR_INVALIDDATA; } @@ -3064,7 +3066,7 @@ static int matroska_parse_laces(MatroskaDemuxContext *matroska, uint8_t **buf, } data += offset; size -= offset; - if (size <= total) { + if (size < total) { return AVERROR_INVALIDDATA; } lace_size[*laces - 1] = size - total; @@ -3524,8 +3526,8 @@ static int matroska_parse_block(MatroskaDemuxContext *matroska, AVBufferRef *buf av_log(matroska->ctx, AV_LOG_INFO, "Invalid stream %"PRIu64"\n", num); return AVERROR_INVALIDDATA; - } else if (size <= 3) - return 0; + } else if (size < 3) + return AVERROR_INVALIDDATA; st = track->stream; if (st->discard >= AVDISCARD_ALL) return res;