[FFmpeg-devel] [PATCH] avformat/demux: Fix segfault due to avcodec_open2 failure (v2)

[Michael Niedermayer]

Hi Pavel

On Tue, Jun 10, 2025 at 08:42:08AM -0600, Pavel Koshevoy wrote:
> On Tue, Jun 10, 2025, 07:39 Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Mon, Jun 09, 2025 at 09:45:28PM -0600, Pavel Koshevoy wrote:
> > > Fixes 'ffprobe 1_poc.mp4' segfault introduced with
> > > commit 0021484d05f9b0f032fa319399de6e24eea0c04f
> > >
> > > codec_close should not assume that the codec_id did not change.
> > > ---
> > >  libavformat/demux.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavformat/demux.c b/libavformat/demux.c
> > > index ecd4f40da9..3749ab67a3 100644
> > > --- a/libavformat/demux.c
> > > +++ b/libavformat/demux.c
> > > @@ -1292,9 +1292,15 @@ static int codec_close(FFStream *sti)
> > >  {
> > >      AVCodecContext *avctx_new = NULL;
> > >      AVCodecParameters *par_tmp = NULL;
> > > +    const AVCodec *new_codec = NULL;
> > >      int ret;
> > >
> > > -    avctx_new = avcodec_alloc_context3(sti->avctx->codec);
> > > +    new_codec =
> > > +      (sti->avctx->codec_id != sti->pub.codecpar->codec_id) ?
> > > +      avcodec_find_decoder(sti->pub.codecpar->codec_id) :
> > > +      sti->avctx->codec;
> > > +
> > > +    avctx_new = avcodec_alloc_context3(new_codec);
> > >      if (!avctx_new) {
> > >          ret = AVERROR(ENOMEM);
> > >          goto fail;
> >
> > This is not about request_probe
> > but about the mpegts demuxer randomly changeing codec id midstream
> >
> 
> 
> I have several real (not crafted like 1_poc.mp4 is) .ts files where codec
> changes from mpeg2video to hevc, from mpeg2audio to eac3 -- while remaining
> on the same PIDs.  I also have .ts files where codec switches between
> mpeg2video and h264.  VLC was able to play such files, but my ffmpeg based
> player (apprenticevideo) could not even see that the codecs changed prior
> to 0021484d05f9b0f032fa319399de6e24eea0c04f.

do these work ?
(work here means the result is a complete file with all frames from the input
 and is playable and seekable)
./ffmpeg -i input.ts -codec copy output.ts
./ffmpeg -i input.ts -codec copy output.mp4
./ffmpeg -i input.ts -vcodec libx264 -acodec libopus output.mkv


> Reverting isn't really an
> option for me, not unless there is a better solution presented.

is adding an exploitable security issue an option for you ?

If people want to keep this, it should be behind a flag and
disabled by default.

Its not enough to fix our code that crashes, other applications
similarly wont expect such id and type changes mid stream


> 
> As I am primarily a public ffmpeg API user -- I am well out of my depth
> when it comes to making non-trivial changes to ffmpegs internals.

Thats ok, but you applied this change to ffmpeg internals, and here
you say "I am well out of my depth when it comes to making non-trivial changes to ffmpegs internals."

Did someone review this ?

commit 0021484d05f9b0f032fa319399de6e24eea0c04f
Author:     Pavel Koshevoy <pkoshevoy at gmail.com>
AuthorDate: Sun May 18 08:57:31 2025 -0600
Commit:     Pavel Koshevoy <pkoshevoy at gmail.com>
CommitDate: Sun May 18 08:57:31 2025 -0600


thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20250610/40a9d16a/attachment.sig>