From patchwork Sun Jul 19 20:47:54 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 21194 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 9E74F44B520 for ; Sun, 19 Jul 2020 23:48:11 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 764C568B982; Sun, 19 Jul 2020 23:48:11 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-ej1-f66.google.com (mail-ej1-f66.google.com [209.85.218.66]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6155468B891 for ; Sun, 19 Jul 2020 23:48:05 +0300 (EEST) Received: by mail-ej1-f66.google.com with SMTP id f12so16028166eja.9 for ; Sun, 19 Jul 2020 13:48:05 -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:mime-version :content-transfer-encoding; bh=gyfG3Mi+ImzIJttsG257PnH7sUwemPq4grUzEXN4DNQ=; b=qIi6wQML8OJS5bQkY86ijgl1ZT38f/JF362av0Xc8LCcm7XHJ/tgoFr4Msl1ud84Ef WIJMZhH+hqI2ppW5fRRX7bk1Rzx4SqJMqpeiurOYh2MJMxTzBLOae1q6hVhr/On3Z/Ya +oQOfY7YG6U1acg2EJIFR8yEoH1M/nAgsYACkvzoBezsz4qCEaAsy8QkE9LXgAMVkYYH HtZaVcbf4S4Iiu9exGDPapZR6G2FiCprtxqP5hnoI+6IvQMI00yK/CkaefY/MDo+/i76 yXTsH9Q/02Tpndp15PNvxJW4E/YtpUSyTTErb7zo7d4Vjnztb4KT9ppG+ZJLvgPFHESQ tt2A== 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:mime-version :content-transfer-encoding; bh=gyfG3Mi+ImzIJttsG257PnH7sUwemPq4grUzEXN4DNQ=; b=Row1x/StmFqtG+HLV52x5arLqw4cYmKSStbA2/plnclwuOSu/MkFSlaEOfePj/l5AC 9R+TEh3GkSL08Tg0ILYJyRgYoBiDMA7eQb/QVX3gBtlPP0oNa9c3bjl5yhjVS7u2t187 QSIACBRBjICXgRFI9JdE+4bYzILzM22ibhOMlEZ5QpLoU2v2InO9KH9EjJiYs4BgxU3Q qF8YQKAVxlRXSS9e10FzP5egAZB0cwspqNZqK3QyqijdZMvCzKBwn+VLf7NA/UgLx35A DLOYEGo806RdApigjDJHAzBFp3fyCkf93j0QFlHcBABY6YvgibVHgaqZzYwFZNQCCOwB 4rNw== X-Gm-Message-State: AOAM532Q5v5u5K4TgmphbPeIV09hEC+uJr1BxwT26XMeeNrSxjiJVQQ/ s5Bna9DH3YLHYxrCrPZ/QzQEXWtZ X-Google-Smtp-Source: ABdhPJynaz5QcIE6XVjiQS+eg0cLKhyQ7GobIFncuImweWk5IT9ZsAXm9mHmZ8dfYcF4GpB1tIun0g== X-Received: by 2002:a17:906:7115:: with SMTP id x21mr17584432ejj.86.1595191684293; Sun, 19 Jul 2020 13:48:04 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc10296.dynamic.kabel-deutschland.de. [188.193.2.150]) by smtp.gmail.com with ESMTPSA id hb8sm13421913ejb.8.2020.07.19.13.48.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 19 Jul 2020 13:48:03 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Sun, 19 Jul 2020 22:47:54 +0200 Message-Id: <20200719204755.32269-1-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH v2 1/2] avformat: Redo cleanup of demuxer upon read_header() failure 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" If reading the header fails, the demuxer's read_close() function (if existing) is not called automatically; instead several demuxers call it via "goto fail" in read_header(). This commit intends to change this by adding a flag to AVInputFormat that can be used to set on a per-AVInputFormat basis whether read_close() should be called generically after an error during read_header(). The flag controlling this behaviour needs to be added because it might be unsafe to call read_close() generally (e.g. this might lead to read_close() being called twice and this might e.g. lead to double-frees if av_free() is used instead of av_freep(); or a size field has not been reset after freeing the elements (see the mov demuxer for an example of this)). Yet the intention is to check and fix all demuxers and make the flag redundant in the medium run. The flag itself is non-public (it resides in libavformat/internal.h), but it has been added to the ordinary (i.e. public) flags field of AVInputFormat, because there is no field for internal flags and adding one is not possible, because libavdevice also defines AVInputFormats and so there is the possibility that a newer libavformat is used together with an older libavdevice that would then lack the new field for internal flags. When it has become redundant, it can be removed again at the next major version bump. Signed-off-by: Andreas Rheinhardt --- This is an updated version of my initial patch [1]. I have also rebased the whole set of patches following it (with the exception of the w3c patch in the next patch they no longer fix a memleak; instead they now only set the flag and remove the manual calls to read_close). Should I resend the other patches, too? [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/258830.html libavformat/internal.h | 6 ++++++ libavformat/utils.c | 11 +++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/libavformat/internal.h b/libavformat/internal.h index 17a6ab07d3..b7441a5959 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -39,6 +39,12 @@ # define hex_dump_debug(class, buf, size) do { if (0) av_hex_dump_log(class, AV_LOG_DEBUG, buf, size); } while(0) #endif +/** Internal flag that is part of AVInputFormat.flags due to + * ABI restrictions that forbid adding a new flags_internal + * to AVInputFormat. */ +#define AVFMT_HEADER_CLEANUP 0x40000000 /**< read_close() should be called + on read_header() failure */ + typedef struct AVCodecTag { enum AVCodecID id; unsigned int tag; diff --git a/libavformat/utils.c b/libavformat/utils.c index 807d9f10cb..2148a03497 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -396,8 +396,12 @@ int av_demuxer_open(AVFormatContext *ic) { if (ic->iformat->read_header) { err = ic->iformat->read_header(ic); - if (err < 0) + if (err < 0) { + if (ic->iformat->read_close && + ic->iformat->flags & AVFMT_HEADER_CLEANUP) + ic->iformat->read_close(ic); return err; + } } if (ic->pb && !ic->internal->data_offset) @@ -624,8 +628,11 @@ FF_ENABLE_DEPRECATION_WARNINGS if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) - if ((ret = s->iformat->read_header(s)) < 0) + if ((ret = s->iformat->read_header(s)) < 0) { + if (s->iformat->flags & AVFMT_HEADER_CLEANUP) + goto close; goto fail; + } if (!s->metadata) { s->metadata = s->internal->id3v2_meta;