Closed Bug 922601 Opened 11 years ago Closed 11 years ago

Signed integer overflow in content/media/MediaStreamGraph.cpp

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox27 + wontfix
firefox28 + wontfix
firefox29 + fixed

People

(Reporter: decoder, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want, testcase, Whiteboard: [qa-][adv-main29-])

Attachments

(2 files, 1 obsolete file)

The following part of our codebase show signed integer overflows at runtime (when running our tests, based on mozilla-inbound 8bf84234319a):

# Format is: Location, Error, Test that triggered it (each in a single line)

content/media/MediaStreamGraph.cpp:386
runtime error: signed integer overflow: 21195 + 9223372036854775807 cannot be represented in type 'long'
file:///builds/slave/test/build/tests/reftest/tests/dom/media/tests/crashtests/791330.html


Signed integer overflows are undefined behavior according to the C/C++ standard and discouraged by the CERT Secure Coding Standard. Even if the overflow itself is intended/checked (which should also be verified), we should not rely on the result, nor on any checks performed with the result after the computation. In the optimization stage, the compiler may assume overflow is not happening and produce wrong results as well as eliminate further checks, recognized as dead code. For further information, see the tracking bug.

Marked as security sensitive until investigated because this overflow is occurring in a range check, not sure what could go wrong here.

Putting needinfo on bnicholson according to hg blame for that particular code. If you're not the right person to take a look, could you please suggest someone? :) Thanks!
Flags: needinfo?(bnicholson)
Hmm...where is my name in the blame? I'm not seeing it anywhere in that file.

I've never touched MediaStreamGraph.cpp before, so I'm not the right person for this. Perhaps roc, given he's that he's touched the majority of it?
Flags: needinfo?(bnicholson) → needinfo?(roc)
(In reply to Brian Nicholson (:bnicholson) from comment #1)
> Hmm...where is my name in the blame? I'm not seeing it anywhere in that file.
> 
> I've never touched MediaStreamGraph.cpp before, so I'm not the right person
> for this. Perhaps roc, given he's that he's touched the majority of it?

That's indeed weird. My hg blame is spilling out revision 2ffeac43b95e (which is yours) but didn't touch the file... HG weirdness or my repo is corrupted...
So I was using git to blame in comment 1...Doing hg blame, I see what you mean. I appear in place of roc, and changeset 2ffeac43b95e, according to blame, touched the majority of this file. wtf?
Attached patch fixSplinter Review
I don't think this is security-sensitive. This calculation only affects whether we decide to send "finished" notifications for a MediaStream.
Assignee: nobody → roc
Attachment #812796 - Flags: review?
Flags: needinfo?(roc)
Group: core-security
Attachment #812796 - Flags: review? → review?(paul)
Attachment #812796 - Flags: review?(paul) → review+
Comment on attachment 812796 [details] [diff] [review]
fix

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

nice debug printfs.  ;-)
https://hg.mozilla.org/mozilla-central/rev/a36ab6c7f174
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Whiteboard: [qa-]
No longer blocks: 943461
Depends on: 943461
Attached patch back out on Aurora (obsolete) — Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 922601
User impact if declined: bug 943461 (camera LED light doesn't turn off)
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): Low risk. This is a backout and the underlying bug isn't very bad
String or IDL/UUID changes made by this patch: none
Attachment #8344413 - Flags: approval-mozilla-aurora?
Comment on attachment 8344413 [details] [diff] [review]
back out on Aurora

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

OK, now we need this on Aurora and Beta.
Attachment #8344413 - Flags: approval-mozilla-beta?
This landed on Firefox 27 which has its first beta coming out at the end of this week.
We desperately need to resolve this (backout) on Aurora and Beta (see bug 943461 for the m-c fix, which is way too big to uplift).  This causes significant user-visible and application-visible regressions that confuse and worry users (camera remains on after it should be off, also can block other apps from using the camera/mic).
Comment on attachment 8344413 [details] [diff] [review]
back out on Aurora

Go ahead with backout - please adjust status flags accordingly, if this stays on central leave 29 fixed.
Attachment #8344413 - Flags: approval-mozilla-beta?
Attachment #8344413 - Flags: approval-mozilla-beta+
Attachment #8344413 - Flags: approval-mozilla-aurora?
Attachment #8344413 - Flags: approval-mozilla-aurora+
Backing this out on Aurora is difficult because bug 938022 part 5 landed in the interim.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Backing this out on Aurora is difficult because bug 938022 part 5 landed in
> the interim.

Ok, what's the solution?  We need to fix bug 943461 (regression caused by landing this) in Aurora/28 as well as 27, and I don't think the 13-part patchset on that bug is upliftable.  Can we rework part 5 of bug 938022?
Flags: needinfo?(roc)
Never was a DOM bug
Component: DOM → Video/Audio
This seems to work. Fixes the LED bug anyway.
https://tbpl.mozilla.org/?tree=Try&rev=555d43fc94e4
Attachment #8344413 - Attachment is obsolete: true
Attachment #8351687 - Flags: review?(rjesup)
Flags: needinfo?(roc)
Attachment #8351687 - Flags: review?(rjesup) → review+
Whiteboard: [qa-] → [qa-][adv-main29-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: