Closed Bug 803086 Opened 7 years ago Closed 7 years ago

Crash in mozilla::MediaPipelineTransmit::ProcessVideoChunk

Categories

(Core :: WebRTC, defect, blocker)

defect
Not set
blocker

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox18 --- unaffected
firefox19 --- verified
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: standard8, Assigned: jesup)

References

Details

(Keywords: crash, csectype-nullptr, regression, Whiteboard: [adv-main19-])

Crash Data

Attachments

(1 file, 2 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-2b8c9fea-f335-40d6-a553-837d42121018 .
============================================================= 

Turning on Video streaming crashes in today's (18th Oct) nightly build.

STR:

1) Visit http://people.mozilla.com/~anarayanan/webrtc/pc_test.html with the relevant prefs set to true.
2) Click Start
3) Share the Camera

=> Nightly Crashes.

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dac5700acf8b&tochange=cb573b9307e5

Top of stack:

0 	XUL 	mozilla::MediaPipelineTransmit::ProcessVideoChunk 	ImageContainer.h:72
1 	XUL 	mozilla::MediaPipelineTransmit::PipelineListener::NotifyQueuedTrackChanges 	MediaPipeline.cpp:538
2 	XUL 	mozilla::MediaStreamGraphImpl::ExtractPendingInput 	MediaStreamGraph.cpp:612
3 	XUL 	mozilla::MediaStreamGraphImpl::RunThread 	MediaStreamGraph.cpp:1392
Attached file callstack (obsolete) —
Even though the testcase is public we should mark a security issue as such.
Group: core-security
Known upstream issue with SRTP, already reported by Christoph.  There's a patch for upstream; I should apply the patch here; it didn't get included when we landed SRTP in m-c.

The read-past-end of this doesn't generate any actual leak of the data and was already noted in the source; so I don't feel this is a security bug.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 790083
This still happens in today's nightly (2012-10-19):

https://crash-stats.mozilla.com/report/index/bp-034ec2e6-8cbb-4e38-a0fe-6d6a12121019

Built with: http://hg.mozilla.org/mozilla-central/rev/0ff60bfb3442 which includes the changeset landed on bug 790083.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 803527
Comment on attachment 672781 [details]
callstack

This callstack is another bug.  The original report is still extant.

See also https://crash-stats.mozilla.com/report/index/bp-5e4ab5ed-7a7f-4586-ac2b-4c65e2121019
Attachment #672781 - Attachment is obsolete: true
This is really wallpaper over the bug until we figure out what and why it's happening, or if NULL images are actually allowed here
Attachment #673221 - Attachment is obsolete: true
Blocks: 801843
Comment on attachment 673243 [details] [diff] [review]
Protect against NULL image chunks in NotifyQueuedTrackChanges()

When I modified getUserMedia to fix video insertion, I took roc's advice to send NULL images until the first one is available (the alternative was to insert a black frame, because otherwise MediaStreams will block until video is up, and audio will queue in the meantime.)  However, apparently there was always an unstated requirement that NotifyQueuedTrackChanges() can give you null images.  This should be documented.

Not easily testable without modifying NotifyPull() for video to force NULL frames.

Only an issue since I landed the patch for bug 801843 which added NULL frames.

We'll want to uplift this with bug 801843
Attachment #673243 - Flags: review?(ekr)
Comment on attachment 673243 [details] [diff] [review]
Protect against NULL image chunks in NotifyQueuedTrackChanges()

Review of attachment 673243 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #673243 - Flags: review?(ekr) → review+
https://hg.mozilla.org/mozilla-central/rev/c20ed5e12b8d
Assignee: nobody → rjesup
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Keywords: verifyme
(In reply to Randell Jesup [:jesup] from comment #6)
> This is really wallpaper over the bug until we figure out what and why it's
> happening, or if NULL images are actually allowed here

Randell, shouldn't we keep this bug open then?
That patch was obsoleted.  Fixed is correct.
Verified on 10/23 build by trying out the test case described in comment 0 a few times. Couldn't reproduce the crash.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [adv-main19-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.