Closed Bug 1486080 Opened Last year Closed Last year

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

Categories

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

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr60 --- fixed
firefox63 --- fixed

People

(Reporter: CE.Mohammad.Alsaleh, Assigned: jya)

References

Details

Attachments

(1 file)

(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
To clarify, the user is still responsible for the data. But allocation/deallocation must be done using av_* functions.
Component: General → Audio/Video: Playback
Product: Firefox → Core
Summary: ffmpeg: libav allocators should be used with mCodecContext->extradata_size → ffmpeg: av_malloc() family of functions should be used with mCodecContext->extradata_size
What's ffmpeg developers argument for changing and breaking their own documentation ?
Flags: needinfo?(CE.Mohammad.Alsaleh)
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 on attachment 9004176 [details]
Bug 1486080 - Always allocate memory to pass extradata. r?padenot

Paul Adenot (:padenot) has approved the revision.
Attachment #9004176 - Flags: review+
Duplicate of this bug: 1485362
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07016c48abb3
Always allocate memory to pass extradata. r=padenot
https://hg.mozilla.org/mozilla-central/rev/07016c48abb3
Status: UNCONFIRMED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee: nobody → jyavenard
The fix should be backported to the 60 ESR channel.
Flags: needinfo?(CE.Mohammad.Alsaleh)
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1490733
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
Attachment #9004176 - Flags: approval-mozilla-esr60?
(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.
> 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.
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 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.
Attachment #9004176 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.