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

[James Almer]

On 7/20/2020 9:35 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 7/19/2020 5:47 PM, Andreas Rheinhardt wrote:
>>> 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. */
>>
>> You can add fields below the "Not public" notice with a minor bump.
>> Nothing is meant to access those directly, except unfortunately lavd.
>> And if you're effectively talking about lavd, then adding it at the end
>> should not affect the offsets of fields currently accessed by indevs. Or
>> am i missing something?
>>
> The point is that it is (unfortunately) allowed to use an older
> libavdevice together with a newer libavformat. This means it is possible
> that the AVInputFormat used in avformat_open_input() may be from a
> libavdevice that is so old that it doesn't have the new internal flags
> field yet. So one either uses an unused bit of the ordinary flags or one
> adds functions that allow to check whether a given AVInputFormat is an
> input device from a too old libavdevice. Or one waits with this change
> until the next major bump and does everything at once.

Wouldn't a check for AV_IS_INPUT_DEVICE(ifmt->priv_class->category)
before trying to look at the new flags_internal field in
avformat_open_input() be enough? Perhaps with an avdevice_version()
check as well in case a device cares about using it. This can be safely
removed after a major bump.

> 
> Notice that in any case, every demuxer with a read_close function needs
> to be checked for compatibility with calling read_close generically on
> read_header error. I have just found two more (besides mov) demuxers
> (rmdec and concat) that are not. Adding this flag allows to easily see
> which demuxers have already been checked.
> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>