From patchwork Thu Sep 5 20:16:03 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 14937 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 6B9EA448AB3 for ; Thu, 5 Sep 2019 23:30:08 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 51E1B687F2B; Thu, 5 Sep 2019 23:30:08 +0300 (EEST) 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 0F36A680A4B for ; Thu, 5 Sep 2019 23:30:02 +0300 (EEST) Received: by mail-wr1-f68.google.com with SMTP id l16so4198864wrv.12 for ; Thu, 05 Sep 2019 13:30:02 -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=WWetvhQN5S9GQoYmTqobMdBgBcXugh1akss2ytm9bIw=; b=C78X4jWR31bSWI4B8W3v7O45A2i1yNIJg4ZFgcGw8rZKDKIEeFvw3VyHEDt8n0Ts7m HhZlqvldmfEHegf5cyoHhjXr3Pshl3OB7nUJEFfsCxiYs89V54isfvrFM30xkxhUHS6B djZOsKeZWZYZvhoZXbL2wwWTe3xHsIlNFsb1WeVmK5a5iKd9yiGjDcy9Xt1xiy2KgCil ZpK07gqdm1R99Kzjs5nmdza16JTCUcMMVhzWm1AxFotvNvUx3dLEJaHZbk3qykoXrIFM 4UUohftBPw7ZYH6s+hjsoDSjPON9l5B1nhMT4CAojZvEJ0rHuuvfWuerF8IXGPBui7Rd 2g6g== 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=WWetvhQN5S9GQoYmTqobMdBgBcXugh1akss2ytm9bIw=; b=uRLApstZlk7T8DJe3CBqFWBp8PNNSCI1CNPFo9xgxpmtFj8h5TIKZKQAmksrjNoIv1 6Yy+/6fTcDS2MPVg95G8ulHGefJUe/dIsRWpeCIqC1OH1RsmehxJWP8D4/VDvfD/gwqH Os8CyHBo4Use4loSZ3spPQylE57lA1sUgo7XoRzLE1I/DEh0KEpc4RQYY+xSwUI/Z0og h8XI12kCJ/Yp8y2z7OVC0Cje9EJnPSnOdZ8y9ciyRq0Z2Lqv5nZikT9CxF6ZUdk+cMC4 HWigu14REa/rweX56vzRjtiOIRJTAiH0ndpFGAGqWgXfUbmioaXuAnZXhm33PXCSJhFk hB0w== X-Gm-Message-State: APjAAAX4JFXtSDoThrVUtmJNw1U+Bvrexw6J5/lw5Z9g1hwjOHaNBY3d 1BarCrAzFNtnN/BqsfoBoiEpGwBpZi4= X-Google-Smtp-Source: APXvYqw3AB6vucLUOo6PAkLmECvndTINlHqsz1wr5PWH+tCr1cvELTtzLrBtG2NdoPQRzVEHRz28mA== X-Received: by 2002:adf:f543:: with SMTP id j3mr4395930wrp.243.1567715045806; Thu, 05 Sep 2019 13:24:05 -0700 (PDT) Received: from localhost.localdomain (ipbcc0f857.dynamic.kabel-deutschland.de. [188.192.248.87]) by smtp.gmail.com with ESMTPSA id g201sm5889769wmg.34.2019.09.05.13.24.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Sep 2019 13:24:05 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Thu, 5 Sep 2019 22:16:03 +0200 Message-Id: <20190905201609.998-2-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20190905201609.998-1-andreas.rheinhardt@gmail.com> References: <20190905201609.998-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 2/8] avformat/matroskadec: Make code more robust 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" 8a286e74 made sure that when a seek is performed to parse a level 1 element, the appropriate level is set (a special function for this purpose is used: matroska_reset_status). This function can fail just like the seeks that it replaced; and just like the seeks that it replaced, it was left unchecked. This became a problem with 865c5370, because with this commit the level has explicitly been used in order to determine how to parse further. This meant that when these seeks failed, the level was not updated. And in certain edge cases it was possible to run into a situation where a call to matroska_parse_cluster was performed when the level was > 2; but this mustn't happen (as one doesn't know how to parse in this situation) and therefore there was an assert for this. The obvious way to check for this is by checking all the calls to matroska_reset_status, i.e. by erroring out if the seek failed and let the generic seeking code take over. Unfortunately, this would break seeking forward in situations where the AVIOContext can only be seeked forward and where no index to seek is available: In this situation matroska_read_seek would first attempt to seek back to the cluster of the last keyframe and begin reading the stream from there for the next keyframe. Said seek would fail in this scenario and parsing would commence from the current position (it works because the level has not been modified yet) until it finds a keyframe whose pts is big enough. So matroska_read_seek works despite the seeking failures. But when one lets the generic seeking code take over, it will try to seek to the last keyframe and fail just as well and the seek itself will fail. So another solution to this problem is necessary. It is surprisingly simple: Instead of asserting that the level is <= 2, resync when it is not; after all, an error must have happened already. Chromium bug 995833 was about this assert; of course, one can no longer run into it after this commit. Signed-off-by: Andreas Rheinhardt --- Honestly, chromium bug 995833 now runs into another bug (a broken pipe (SIGPIPE)) outside of the FFmpeg related code. libavformat/matroskadec.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 0c881a67e8..47386b2631 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -3676,7 +3676,13 @@ static int matroska_parse_cluster(MatroskaDemuxContext *matroska) MatroskaBlock *block = &cluster->block; int res; - av_assert0(matroska->num_levels <= 2); + if (matroska->num_levels > 2) { + /* This can only happen if a reset has failed. + * So resync now in order to be able to parse again. */ + res = matroska_resync(matroska, avio_tell(matroska->ctx->pb)); + if (res < 0) + return res; + } if (matroska->num_levels == 1) { res = ebml_parse(matroska, matroska_segment, NULL);