[FFmpeg-devel,04/21] avformat: Redo cleanup of demuxer upon read_header() failure

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 has been 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() instead of av_freep() is used; 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 has been added to a new internal field of AVInputFormat,
flags_internal. When it has become redundant, it can be removed again
at the next major version bump.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/avformat.h |  7 +++++++
 libavformat/utils.c    | 11 +++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

On 3/22/2020 12:47 AM, 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 has been 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() instead of av_freep() is used; 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 has been added to a new internal field of AVInputFormat,
> flags_internal. When it has become redundant, it can be removed again
> at the next major version bump.

Fields considered not public (And thus added below the relevant notice
in the struct) can be added and removed at any time. So this sentence is
not needed.

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/avformat.h |  7 +++++++
>  libavformat/utils.c    | 11 +++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 9b9b634ec3..50b90788b1 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -780,6 +780,13 @@ typedef struct AVInputFormat {
>       * @see avdevice_capabilities_free() for more details.
>       */
>      int (*free_device_capabilities)(struct AVFormatContext *s, struct AVDeviceCapabilitiesQuery *caps);
> +
> +#define FF_INPUTFORMAT_HEADER_CLEANUP 0x0001 /**< read_close() should be called
> +                                                  on read_header() failure */

If this is internal, then it should be defined in internal.h, same as
the caps_internal flags for AVCodecContext.

> +    /**
> +     * Can use flags: FF_INPUTFORMAT_HEADER_CLEANUP
> +     */
> +    int flags_internal;
>  } AVInputFormat;
>  /**
>   * @}
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index a58e47fabc..87807a7841 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -400,8 +400,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_internal & FF_INPUTFORMAT_HEADER_CLEANUP)
> +                ic->iformat->read_close(ic);
>              return err;
> +        }
>      }
>  
>      if (ic->pb && !ic->internal->data_offset)
> @@ -628,8 +632,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_internal & FF_INPUTFORMAT_HEADER_CLEANUP)
> +                goto close;
>              goto fail;
> +        }
>  
>      if (!s->metadata) {
>          s->metadata = s->internal->id3v2_meta;
>

James Almer:
> On 3/22/2020 12:47 AM, 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 has been 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() instead of av_freep() is used; 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 has been added to a new internal field of AVInputFormat,
>> flags_internal. When it has become redundant, it can be removed again
>> at the next major version bump.
> 
> Fields considered not public (And thus added below the relevant notice
> in the struct) can be added and removed at any time. So this sentence is
> not needed.
> 
This field is not public, but I thought that the fact that libavdevice
also defines AVInputFormats makes this de-facto avpriv. Isn't it allowed
to use a newer libavdevice together with an older libavformat? Imagine
your libavdevice to be so new that the field has already been removed,
yet the libavformat being so old that it still checks for the field.
That would not go well.
(If I am right then this gives the task of checking and adapting all the
demuxers a certain urgency to be finished before the next major bump.)

>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavformat/avformat.h |  7 +++++++
>>  libavformat/utils.c    | 11 +++++++++--
>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 9b9b634ec3..50b90788b1 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -780,6 +780,13 @@ typedef struct AVInputFormat {
>>       * @see avdevice_capabilities_free() for more details.
>>       */
>>      int (*free_device_capabilities)(struct AVFormatContext *s, struct AVDeviceCapabilitiesQuery *caps);
>> +
>> +#define FF_INPUTFORMAT_HEADER_CLEANUP 0x0001 /**< read_close() should be called
>> +                                                  on read_header() failure */
> 
> If this is internal, then it should be defined in internal.h, same as
> the caps_internal flags for AVCodecContext.
> 
I thought about this and given that avformat.h already defines
FF_DEBUG_TS I put it in there. But sure, I can move it.

- Andreas

On Sun, Mar 22, 2020 at 04:47:39AM +0100, 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 has been 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() instead of av_freep() is used; 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 has been added to a new internal field of AVInputFormat,
> flags_internal. When it has become redundant, it can be removed again
> at the next major version bump.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/avformat.h |  7 +++++++
>  libavformat/utils.c    | 11 +++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 9b9b634ec3..50b90788b1 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -780,6 +780,13 @@ typedef struct AVInputFormat {
>       * @see avdevice_capabilities_free() for more details.
>       */
>      int (*free_device_capabilities)(struct AVFormatContext *s, struct AVDeviceCapabilitiesQuery *caps);
> +
> +#define FF_INPUTFORMAT_HEADER_CLEANUP 0x0001 /**< read_close() should be called
> +                                                  on read_header() failure */
> +    /**
> +     * Can use flags: FF_INPUTFORMAT_HEADER_CLEANUP
> +     */
> +    int flags_internal;
>  } AVInputFormat;
>  /**
>   * @}

This feature is a good idea.

thx

[...]

Andreas Rheinhardt:
> James Almer:
>> On 3/22/2020 12:47 AM, 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 has been 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() instead of av_freep() is used; 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 has been added to a new internal field of AVInputFormat,
>>> flags_internal. When it has become redundant, it can be removed again
>>> at the next major version bump.
>>
>> Fields considered not public (And thus added below the relevant notice
>> in the struct) can be added and removed at any time. So this sentence is
>> not needed.
>>
> This field is not public, but I thought that the fact that libavdevice
> also defines AVInputFormats makes this de-facto avpriv. Isn't it allowed
> to use a newer libavdevice together with an older libavformat? Imagine
> your libavdevice to be so new that the field has already been removed,
> yet the libavformat being so old that it still checks for the field.
> That would not go well.
> (If I am right then this gives the task of checking and adapting all the
> demuxers a certain urgency to be finished before the next major bump.)
> 
I reconsidered this and I think I got it exactly wrong: libavdevice is
linked against libavformat, not the other way around. Therefore one can
swap the libavformat against a newer one, but one is not allowed to
update libavdevice without also updating libavformat. (This presumes
that the usual rules apply in this scenario; I haven't seen any
exception documented for the special case of the heavily interwoven
libavdevice and libavformat, so I guess there is none.)

This means that adding new fields to AVInputFormat (and using them) is
not possible, because said AVInputFormat might come from an older
version of libavdevice and might therefore not have these fields.

This means that my proposal has to be modified. flags_internal is not
feasible (unless one uses a version check for. One could use a not
publically documented flag of the normal flags.

This also means that this flag can not be enabled by default (and
therefore removed) before the next major version bump. E.g. the alsa
input device seems to be incompatible with automatically running its
read_close function after failure of reading the header (the current
error handling closes a snd_pcm_t, but doesn't reset the pointer to NULL).

- Andreas

Andreas Rheinhardt (12020-03-24):
> I reconsidered this and I think I got it exactly wrong: libavdevice is
> linked against libavformat, not the other way around. Therefore one can
> swap the libavformat against a newer one, but one is not allowed to
> update libavdevice without also updating libavformat. (This presumes
> that the usual rules apply in this scenario; I haven't seen any
> exception documented for the special case of the heavily interwoven
> libavdevice and libavformat, so I guess there is none.)
> 
> This means that adding new fields to AVInputFormat (and using them) is
> not possible, because said AVInputFormat might come from an older
> version of libavdevice and might therefore not have these fields.

I would like to emphasize that this whole issue is again a drawback of
having the libraries reside in separate shared objects. A situation for
which I have yet to be told a single actual benefit.

Regards,