| Message ID | 20200719204755.32269-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/2] avformat: Redo cleanup of demuxer upon read_header() failure | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Sun, Jul 19, 2020 at 10:47:54PM +0200, 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@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(-) LGTM thx [...]
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@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? > +#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; >
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@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. 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
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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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;
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@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(-)