Closed Bug 1240763 Opened 10 years ago Closed 10 years ago

Crash with possible use-after-free [@ nsPresContext::MediaFeatureValuesChanged ]

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
45 Branch
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1233259
Tracking Flags:
Tracking Status
firefox43 --- affected
firefox44 --- wontfix
firefox45 --- fixed
firefox-esr38 --- unaffected
firefox-esr45 --- fixed

People

(Reporter: johns, Unassigned)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main45-] port fix from 1233259 for pre-45 releases.)

Crash Data

Via https://crash-stats.mozilla.com/report/index/d9af11a1-829d-44e1-bc71-785c62160119 Looks like mDocument could be gone at this point. Marking sec flag because UAF. Possibly related to <picture> work landing recently, which uses this callback
This is curious, because there are other uses of mDocument in nsPresContext::MediaFeatureValuesChanged that existed before my addition of mDocument->NotifyMediaFeatureValuesChanged(), yet that is the line that is triggering the crash.
Actually, I think this is bug 1233259. All the crashes are for FF 43 (where 133259 wasn't uplifted to beta), and there's one for a FF 45 build from last November.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Sorry, mis-CCed Ehsan.
I don't think this is a dup. bug 1233259 explicitly patched code which was added to FF45.
Ok, so this bug is likely triggered by a similar codepath: https://hg.mozilla.org/releases/mozilla-release/file/tip/dom/html/HTMLImageElement.cpp#l592 . This was moved in bug 1166138 and patched in bug 1233259, so if we want to address this in pre-FF 45 versions we should backport the fix from bug 1233259 to the old code linked previously.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
We're about to ship Firefox 44 so it's probably not worth a stop-ship-rebuild there. Does this affect ESR-38? The code linked to in comment 5 looks like the equivalent ESR38 code: https://hg.mozilla.org/releases/mozilla-esr38/file/tip/dom/html/HTMLImageElement.cpp#l595
Assignee: nobody → bugs
Group: core-security → dom-core-security
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox45: --- → fixed
status-firefox-esr38: --- → ?
status-firefox-esr45: --- → fixed
Depends on: 1233259
Flags: needinfo?(bugs)
Keywords: csectype-uaf, sec-high
Whiteboard: port fix from 1233259 for pre-45 releases.
Group: dom-core-security → layout-core-security
This does not affect ESR 38.
let me look at the beta code. I have still no idea why we're crashing. bug 1233259 fixed explicitly a case which bug 1166138 added.
So the crash-stat in comment 0 is from FF45 before bug 1233259 was fixed. Josh, which FF43 crashes you were talking about in comment 2? As far as I see, AddResponsiveContent/RemoveResponsiveContent usage in HTMLImageElement is safe in FF44, and those methods aren't being called anywhere else
Flags: needinfo?(bugs) → needinfo?(josh)
But that FF43 crash is in totally different place. The FF45/46 bug was about NotifyMediaFeatureValuesChanged(); call, FF43 is mql->MediumFeaturesChanged(notifyList);
dbaron, the FF43 crash from comment 10 might be something you have an idea about.
Flags: needinfo?(dbaron)
(clearing 'assigned to' since I have nothing I could do here atm.)
Assignee: bugs → nobody
The top stack frame in the comment 10 crash seems bogus. I don't see a way to get from PresShell::UpdateCanvasBackground to nsPresContext::MediaFeatureValuesChanged. http://hg.mozilla.org/releases/mozilla-release/annotate/146f494b6a79/layout/base/nsPresShell.cpp#l5284 I think it can be ignored since there's only a single instance with that stack.
Looks like we're done here.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?(josh)
Resolution: --- → DUPLICATE
Sounds like you don't need anything from me now. (Please re-needinfo if I'm wrong.)
Flags: needinfo?(dbaron)
Hi Dan, this bug indicates esr38 is affected but was resolved as a dup of bug 1233259 which is marked esr38 affected. Do I need to get the fix from that bug uplifted to esr38? Please let me know. Thanks!
Flags: needinfo?(dveditz)
There is likely another bug lurking in here: I see two more UAF crashes on mql->MediumFeaturesChanged(notifyList) as in comment 10 / 11, both reading from a poisoned address. One is on 44.0.2 but bp-76eef7c5-65f1-4477-8720-1f6282160218 is on 45.0b6 so it has the fix from bug 1233259. No evidence of ESR 38 crashes but that's an older part of this routine and we can't rule it out unless we discover the underlying cause. But since that's a different problem than the one that sparked this bug we should leave this a dupe and file a new bug for that stack.
status-firefox-esr38: affected → unaffected
tracking-firefox-esr38: 45+ → ---
Flags: needinfo?(dveditz)
Depends on: 1252206
(In reply to Daniel Veditz [:dveditz] from comment #18) > we should leave this a dupe and file a new bug for that stack. I filed bug 1252206 for that.
Whiteboard: port fix from 1233259 for pre-45 releases. → [adv-main45-] port fix from 1233259 for pre-45 releases.
Group: layout-core-security
You need to log in before you can comment on or make changes to this bug.