| Message ID | 20240430004854.199741-3-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/kvag: Check sample_rate | expand |
On 4/29/2024 9:48 PM, Michael Niedermayer wrote: > Fixes: division by 0 > Fixes: decoder modifying demuxer channels on failure > Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3 > > Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavutil/opt.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/libavutil/opt.c b/libavutil/opt.c > index ecbf7efe5fb..24c08e4bc06 100644 > --- a/libavutil/opt.c > +++ b/libavutil/opt.c > @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr) > av_dict_free((AVDictionary **)ptr); > break; > > - case AV_OPT_TYPE_CHLAYOUT: > + case AV_OPT_TYPE_CHLAYOUT: { > + int nb_channels = ((AVChannelLayout *)ptr)->nb_channels; > av_channel_layout_uninit((AVChannelLayout *)ptr); > - break; > + ((AVChannelLayout *)ptr)->nb_channels = nb_channels; > + break;} > > default: > break; A little bit of context would be helpful here. What's using nb_channels after av_opt_free was called and where?
On Tue, Apr 30, 2024 at 06:27:23PM -0300, James Almer wrote: > On 4/29/2024 9:48 PM, Michael Niedermayer wrote: > > Fixes: division by 0 > > Fixes: decoder modifying demuxer channels on failure > > Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3 > > > > Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavutil/opt.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/libavutil/opt.c b/libavutil/opt.c > > index ecbf7efe5fb..24c08e4bc06 100644 > > --- a/libavutil/opt.c > > +++ b/libavutil/opt.c > > @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr) > > av_dict_free((AVDictionary **)ptr); > > break; > > - case AV_OPT_TYPE_CHLAYOUT: > > + case AV_OPT_TYPE_CHLAYOUT: { > > + int nb_channels = ((AVChannelLayout *)ptr)->nb_channels; > > av_channel_layout_uninit((AVChannelLayout *)ptr); > > - break; > > + ((AVChannelLayout *)ptr)->nb_channels = nb_channels; > > + break;} > > default: > > break; > > A little bit of context would be helpful here. What's using nb_channels > after av_opt_free was called and where? demuxer sets nb_channels find stream info copies codec params to context find stream info tries opening decoder decoder, refuses, and opt_free_elem() is called on cleanup context now has 0 channels context gets copied into params of demuxer demuxer goes like i have set the channels to a non zero value let me devide by them and oops there is more than one position in this chain of events this can be fixed thx [...]
On 4/30/2024 9:38 PM, Michael Niedermayer wrote: > On Tue, Apr 30, 2024 at 06:27:23PM -0300, James Almer wrote: >> On 4/29/2024 9:48 PM, Michael Niedermayer wrote: >>> Fixes: division by 0 >>> Fixes: decoder modifying demuxer channels on failure >>> Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3 >>> >>> Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavutil/opt.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavutil/opt.c b/libavutil/opt.c >>> index ecbf7efe5fb..24c08e4bc06 100644 >>> --- a/libavutil/opt.c >>> +++ b/libavutil/opt.c >>> @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr) >>> av_dict_free((AVDictionary **)ptr); >>> break; >>> - case AV_OPT_TYPE_CHLAYOUT: >>> + case AV_OPT_TYPE_CHLAYOUT: { >>> + int nb_channels = ((AVChannelLayout *)ptr)->nb_channels; >>> av_channel_layout_uninit((AVChannelLayout *)ptr); >>> - break; >>> + ((AVChannelLayout *)ptr)->nb_channels = nb_channels; >>> + break;} >>> default: >>> break; >> >> A little bit of context would be helpful here. What's using nb_channels >> after av_opt_free was called and where? > > demuxer sets nb_channels > find stream info copies codec params to context > find stream info tries opening decoder > decoder, refuses, and opt_free_elem() is called on cleanup > context now has 0 channels > context gets copied into params of demuxer Should this happen if the internal decoder failed to generate a frame? Can it be avoided? > demuxer goes like i have set the channels to a non zero value let me devide by them > and oops > > there is more than one position in this chain of events this can be fixed > > thx > > [...] > > > _______________________________________________ > 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".
On 4/30/2024 9:38 PM, Michael Niedermayer wrote: > On Tue, Apr 30, 2024 at 06:27:23PM -0300, James Almer wrote: >> On 4/29/2024 9:48 PM, Michael Niedermayer wrote: >>> Fixes: division by 0 >>> Fixes: decoder modifying demuxer channels on failure >>> Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3 >>> >>> Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavutil/opt.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavutil/opt.c b/libavutil/opt.c >>> index ecbf7efe5fb..24c08e4bc06 100644 >>> --- a/libavutil/opt.c >>> +++ b/libavutil/opt.c >>> @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr) >>> av_dict_free((AVDictionary **)ptr); >>> break; >>> - case AV_OPT_TYPE_CHLAYOUT: >>> + case AV_OPT_TYPE_CHLAYOUT: { >>> + int nb_channels = ((AVChannelLayout *)ptr)->nb_channels; >>> av_channel_layout_uninit((AVChannelLayout *)ptr); >>> - break; >>> + ((AVChannelLayout *)ptr)->nb_channels = nb_channels; >>> + break;} >>> default: >>> break; >> >> A little bit of context would be helpful here. What's using nb_channels >> after av_opt_free was called and where? > > demuxer sets nb_channels > find stream info copies codec params to context > find stream info tries opening decoder > decoder, refuses, and opt_free_elem() is called on cleanup > context now has 0 channels > context gets copied into params of demuxer > demuxer goes like i have set the channels to a non zero value let me devide by them > and oops > > there is more than one position in this chain of events this can be fixed > > thx I think we should prevent avcodec_open2() from clearing a user-set parameter when closing, rather than change how av_opt_free works. Like so: > 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);
diff --git a/libavutil/opt.c b/libavutil/opt.c index ecbf7efe5fb..24c08e4bc06 100644 --- a/libavutil/opt.c +++ b/libavutil/opt.c @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr) av_dict_free((AVDictionary **)ptr); break; - case AV_OPT_TYPE_CHLAYOUT: + case AV_OPT_TYPE_CHLAYOUT: { + int nb_channels = ((AVChannelLayout *)ptr)->nb_channels; av_channel_layout_uninit((AVChannelLayout *)ptr); - break; + ((AVChannelLayout *)ptr)->nb_channels = nb_channels; + break;} default: break;
Fixes: division by 0 Fixes: decoder modifying demuxer channels on failure Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3 Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavutil/opt.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)