| Message ID | 20240501190156.36095-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/avcodec: prevent ch_layout from being uninitialized in ff_codec_close() | expand |
James Almer: > It's a user-set parameter shared with AVCodecParameters, so it should only > be freed by avcodec_free_context(). > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avcodec.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c > index 888dd76228..fc8a40e4db 100644 > --- a/libavcodec/avcodec.c > +++ b/libavcodec/avcodec.c > @@ -414,6 +414,7 @@ void avsubtitle_free(AVSubtitle *sub) > > av_cold void ff_codec_close(AVCodecContext *avctx) > { > + AVChannelLayout ch_layout; > int i; > > if (!avctx) > @@ -468,7 +469,13 @@ av_cold void ff_codec_close(AVCodecContext *avctx) > > if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) > av_opt_free(avctx->priv_data); > + > + // Work around av_opt_free() unsetting ch_layout > + ch_layout = avctx->ch_layout; > + memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout)); > av_opt_free(avctx); > + avctx->ch_layout = ch_layout; > + > av_freep(&avctx->priv_data); > if (av_codec_is_encoder(avctx->codec)) { > av_freep(&avctx->extradata); This and the other patches will cause memleaks for users that use allocated channel layouts and avcodec_close()+av_free() (this is deprecated, not forbidden). Furthermore, where does the rule "user-set parameters shared with AVCodecParameters should only be freed by avcodec_free_context()" come from? It is news to me. - Andreas
On 5/1/2024 5:26 PM, Andreas Rheinhardt wrote: > James Almer: >> It's a user-set parameter shared with AVCodecParameters, so it should only >> be freed by avcodec_free_context(). >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/avcodec.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >> index 888dd76228..fc8a40e4db 100644 >> --- a/libavcodec/avcodec.c >> +++ b/libavcodec/avcodec.c >> @@ -414,6 +414,7 @@ void avsubtitle_free(AVSubtitle *sub) >> >> av_cold void ff_codec_close(AVCodecContext *avctx) >> { >> + AVChannelLayout ch_layout; >> int i; >> >> if (!avctx) >> @@ -468,7 +469,13 @@ av_cold void ff_codec_close(AVCodecContext *avctx) >> >> if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) >> av_opt_free(avctx->priv_data); >> + >> + // Work around av_opt_free() unsetting ch_layout >> + ch_layout = avctx->ch_layout; >> + memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout)); >> av_opt_free(avctx); >> + avctx->ch_layout = ch_layout; >> + >> av_freep(&avctx->priv_data); >> if (av_codec_is_encoder(avctx->codec)) { >> av_freep(&avctx->extradata); > > This and the other patches will cause memleaks for users that use > allocated channel layouts and avcodec_close()+av_free() (this is > deprecated, not forbidden). That's awful, but guess it needs to be supported until avcodec_close() is gone, so I'm withdrawing this patch. > > Furthermore, where does the rule "user-set parameters shared with > AVCodecParameters should only be freed by avcodec_free_context()" come > from? It is news to me. It's not a rule, it's the ideal/expected behavior seeing the crash Michael found, where the only shared field cleared during avcodec_close() was ch_layout because it may contain allocated data and can be set through an AVOption. If you're copying params between codecpar and avctx, the latter should not have only one of the relevant fields nuked on an internal failure.
diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c index 888dd76228..fc8a40e4db 100644 --- a/libavcodec/avcodec.c +++ b/libavcodec/avcodec.c @@ -414,6 +414,7 @@ void avsubtitle_free(AVSubtitle *sub) av_cold void ff_codec_close(AVCodecContext *avctx) { + AVChannelLayout ch_layout; int i; if (!avctx) @@ -468,7 +469,13 @@ av_cold void ff_codec_close(AVCodecContext *avctx) if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) av_opt_free(avctx->priv_data); + + // Work around av_opt_free() unsetting ch_layout + ch_layout = avctx->ch_layout; + memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout)); av_opt_free(avctx); + avctx->ch_layout = ch_layout; + av_freep(&avctx->priv_data); if (av_codec_is_encoder(avctx->codec)) { av_freep(&avctx->extradata);
It's a user-set parameter shared with AVCodecParameters, so it should only be freed by avcodec_free_context(). Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avcodec.c | 7 +++++++ 1 file changed, 7 insertions(+)