1486080 - ffmpeg: av_malloc() family of functions should be used with mCodecContext->extradata_size

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Mohammad AlSaleh]

(discussed in #ffmpeg-devel@freenode)

Starting with this FFmpeg commit:
https://git.videolan.org/?p=ffmpeg.git;a=commit;h=f631c328e680a3dd491936b92f69970c20cdcfc7

Firefox's media playback thread crashes in the AAC decoding context.

The commit adds a call to avcodec_parameters_to_context().

That function calls av_freep(&codec->extradata):
https://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/utils.c;h=285bfdbc63cb4311806850166081194ae8b2fa4c;hb=f631c328e680a3dd491936b92f69970c20cdcfc7#l2133

This means that extradata is now assumed to be allocated by libav
allocators, even if the context is a decoding one, despite the documentation
saying otherwise:
https://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/avcodec.h;h=31e50d5a947df2237a293c2db83e2d345ae5c623;hb=HEAD#l1619


That call to av_freep() is behind the crashes.

A MediaByteBuffer can't own extradata anymore:
https://hg.mozilla.org/mozilla-central/file/tip/dom/media/platforms/ffmpeg/FFmpegDataDecoder.cpp#l85

Comment 1

[Mohammad AlSaleh]

To clarify, the user is still responsible for the data. But allocation/deallocation must be done using av_* functions.

Comment 2

[Jean-Yves Avenard [:jya]]

What's ffmpeg developers argument for changing and breaking their own documentation ?

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1486080 - Always allocate memory to pass extradata. r?padenot

Despite wording of the documentation to the contrary, we can't provide a static pointer to an immutable object.
Apparently, it's always been that way.

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 9004176 [details]
Bug 1486080 - Always allocate memory to pass extradata. r?padenot

Paul Adenot (:padenot) has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07016c48abb3
Always allocate memory to pass extradata. r=padenot

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/07016c48abb3

Comment 8

[Mohammad AlSaleh]

The fix should be backported to the 60 ESR channel.

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 9004176 [details]
Bug 1486080 - Always allocate memory to pass extradata. r?padenot

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: firefox will crash when playing AAC audio when using ffmepg 4.0
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): 63
String or UUID changes made by this patch: none

Comment 11

[James Almer]

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> What's ffmpeg developers argument for changing and breaking their own
> documentation ?

The argument would be "The implications of the change were not properly assessed during the review phase". So basically, no one was aware it was breaking anything.
I have sent a patch to get the problematic commit reverted. It is effectively an API break to internally free and essentially attempt to take ownership of avctx->extradata in this situation. Fortunately, the commit is not part of any stable release.

I see Mohammad AlSaleh reported this issue to the ffmpeg mailing list, but didn't reply when he was asked for more information, so nothing came out of it. It would have also been ideal to instead report the issue to the bug tracker, and chances are it would have been looked at and dealt with in a more timely manner.

Comment 12

[Mohammad AlSaleh]

> I see Mohammad AlSaleh reported this issue to the ffmpeg mailing list, but didn't reply when he was asked for more information, so nothing came out of it. It would have also been ideal to instead report the issue to the bug tracker, and chances are it would have been looked at and dealt with in a more timely manner.

I only had time to bisect that day. I didn't know what was the real issue. I did however
follow up the next day on IRC when I had more time.

Check the logs here (starting from 17:59:50 CEST):
https://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/2018-August/005220.html

It is FFmpeg developers who told me to report this issue here.

A part of the larger issue here IMHO is that C is not Rust. *Own* does not have
a precise agreed upon definition. Firefox developers assumed *owning*
means they should provide libavcodec with an *immutable view* into
extradata. FFMpeg developers assumed it only meant the library user is
responsible for the initial allocation and final deallocation, using
the av_malloc() family of functions.

Whatever is decided here, API docs will need to be a lot more precise in the future,
so users who write code in languages like modern C++ or Rust can understand
the *exact* constraints implied.

Comment 13

[Mohammad AlSaleh]

It appears the situation will not change in FFmpeg. So the fix still needs to be backported:
https://ffmpeg.org/pipermail/ffmpeg-devel/2018-September/234210.html

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9004176 [details]
Bug 1486080 - Always allocate memory to pass extradata. r?padenot

Fixes a crash with ffmpeg 4.0. Approved for ESR 60.3.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/3f8d57d936ea