OpenH264 calls GMPVideoDecoderCallback after DecodingComplete -- uses freed memory

RESOLVED FIXED

Status

External Software Affecting Firefox
OpenH264
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: tsmith, Assigned: Hank Peng)

Tracking

(Blocks: 1 bug, {crash, csectype-bounds, sec-high})

unspecified
x86_64
Linux
crash, csectype-bounds, sec-high
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 affected, firefox45 affected)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Created attachment 8660845 [details]
call_stack.txt
Anthony, this is a problem with GMP, can you have a look ?
Component: Audio/Video: MSG/cubeb/GMP → Audio/Video: Playback
Flags: needinfo?(ajones)
Ethan, can you look at this as well and verify if it's known?
Flags: needinfo?(ethanhugg)

Comment 3

3 years ago
This is not the same stack I see with the OpenH264 refcount bug, but if this was created by testing WebRTC/H264, then it would be worthwhile to try with a build off the master branch to see if it's also repeatable there.
Flags: needinfo?(ethanhugg)
I'm guessing this is a WebRTC or at least general GMP issue on the basis that we're not using GMP on Linux for media playback.

Chris - who is the right person to look into this?
Flags: needinfo?(cpearce)
This is a bug in OpenH264.

An OpenH264 task to run the GMPVideoDecoderCallback::InputDataExhausted() callback is running after the GMPVideoDecoder::DecodingComplete() implementation has run, and the GMPVideoDecoderCallback's memory has been freed.

OpenH264's GMPVideoDecoder::DecodingComplete() implementation needs to set a flag that ensures that nothing ever tries to use the GMPVideoDecoderCallback after GMPVideoDecoder::DecodingComplete() is called.

That's what gmp-clearkey and Adobe's GMP do; we set a flag here:
https://dxr.mozilla.org/mozilla-central/source/media/gmp-clearkey/0.1/VideoDecoder.cpp#421

And we check the flag inside our GMPTask wrapper here:
https://dxr.mozilla.org/mozilla-central/source/media/gmp-clearkey/0.1/VideoDecoder.cpp#440
Flags: needinfo?(cpearce)
The alternative fix in Firefox would be for us to never de-allocate the thing that implements GMPVideoDecoderCallback. That's not ideal, as we'd be leaking memory then.

Updated

3 years ago
Depends on: 1170319
Group: media-core-security → core-security-release
Component: Audio/Video: Playback → OpenH264
Keywords: sec-high
Product: Core → Plugins
Summary: ASan: heap-buffer-overflow [@mozilla::gmp::GMPVideoDecoderParent::ActorDestroy] → OpenH264 calls GMPVideoDecoderCallback after DecodingComplete -- uses freed memory
Version: Trunk → unspecified
Flags: needinfo?(ajones)
status-firefox44: --- → affected

Updated

3 years ago
Assignee: nobody → haibozhu
(Assignee)

Updated

3 years ago
Assignee: haibozhu → hankpeng
(Assignee)

Comment 7

3 years ago
Move to next openh264 release
Depends on: 1217114
No longer depends on: 1170319
Will this be fixed in 1.5? I see no progress here.
status-firefox45: --- → affected
(Assignee)

Comment 9

3 years ago
(In reply to Al Billings [:abillings] from comment #8)
> Will this be fixed in 1.5? I see no progress here.

Not in the scope of 1.5. Should be released after 1.5. I should be able to provide a fix for code review soon.
(Assignee)

Comment 10

3 years ago
Code review for openh264 fix: https://rbcommons.com/s/OpenH264/r/1366/diff/1#index_header
(Assignee)

Comment 11

3 years ago
The fix was merged into the openh264 master branch with commit 955fce6 (https://github.com/cisco/openh264/commit/955fce60a1cfcbd66a778350fc4b6d8e54ea946f).
Group: media-core-security
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Group: media-core-security
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.