[FFmpeg-devel] [PATCH v2 1/2] avformat: Redo cleanup of demuxer upon read_header() failure

[Andreas Rheinhardt]

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 <andreas.rheinhardt at gmail.com>
---
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;
-- 
2.20.1