[FFmpeg-devel] [PATCH 1/6] Revert "avcodec: add FF_CODEC_CAP_INIT_CLEANUP for all codecs which use ff_mpv_common_init()"

[Michael Niedermayer]

On Mon, Apr 05, 2021 at 03:28:52AM +0200, Andreas Rheinhardt wrote:
> From: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> 
> This mostly reverts commit 4b2863ff01b1fe93d9a518523c9098d17a9d8c6f.
> Said commit removed the freeing code from ff_mpv_common_init(),
> ff_mpv_common_frame_size_change() and ff_mpeg_framesize_alloc() and
> instead added the FF_CODEC_CAP_INIT_CLEANUP to several codecs that use
> ff_mpv_common_init(). This introduced several bugs:
> 
> a) Several decoders using ff_mpv_common_init() in their init function were
> forgotten: This affected FLV, Intel H.263, RealVideo 3.0 and V4.0 as well as
> VC-1/WMV3.
> b) ff_mpv_common_init() is not only called from the init function of
> codecs, it is also called from AVCodec.decode functions. If an error
> happens after an allocation has succeeded, it can lead to memleaks;
> furthermore, it is now possible for the MpegEncContext to be marked as
> initialized even when ff_mpv_common_init() returns an error and this can
> lead to segfaults because decoders that call ff_mpv_common_init() when
> decoding a frame can mistakenly think that the MpegEncContext has been
> properly initialized. This can e.g. happen with H.261 or MPEG-4.
> c) Removing code for freeing from ff_mpeg_framesize_alloc() (which can't
> be called from any init function) can lead to segfaults because the
> check for whether it needs to allocate consists of checking whether the
> first of the buffers allocated there has been allocated. This part has
> already been fixed in 76cea1d2ce3f23e8131c8664086a1daf873ed694.
> d) ff_mpv_common_frame_size_change() can also not be reached from any
> AVCodec.init function; yet the changes can e.g. lead to segfaults with
> decoders using ff_h263_decode_frame() upon allocation failure, because
> the MpegEncContext will upon return be flagged as both initialized and
> not in need of reinitialization (granted, the fact that
> ff_h263_decode_frame() clears context_reinit before the context has been
> reinited is a bug in itself). With the earlier version, the context
> would be cleaned upon failure and it would be attempted to initialize
> the context again in the next call to ff_h263_decode_frame().
> 
> While a) could be fixed by adding the missing FF_CODEC_CAP_INIT_CLEANUP,
> keeping the current approach would entail adding cleanup code to several
> other places because of b). Therefore ff_mpv_common_init() is again made
> to clean up after itself; the changes to the wmv2 decoder and the SVQ1
> encoder have not been reverted: The former fixed a memleak, the latter
> allowed to remove cleanup code.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
>  libavcodec/h261dec.c       |  2 +-
>  libavcodec/h263dec.c       |  4 ++--
>  libavcodec/mpeg12dec.c     |  9 +++++----
>  libavcodec/mpeg4videodec.c |  3 +--
>  libavcodec/mpegvideo.c     | 31 ++++++++++++++++++++-----------
>  libavcodec/msmpeg4dec.c    |  8 ++++----
>  libavcodec/rv10.c          |  2 --
>  7 files changed, 33 insertions(+), 26 deletions(-)

whole patchset LGTM
also seems to work fine

please add mentionings of any CISPA samples this fixes so that is kept track
of. (assuming you succeded reproducing the issues or know/can guess reasonably
well which patch fixed what)

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210407/caec4956/attachment.sig>