Closed Bug 1240763 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

45 Branch
x86_64
macOS
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1233259
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: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1233259
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
Depends on: 1233259
Flags: needinfo?(bugs)
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: 4 years ago4 years ago
Flags: needinfo?(josh)
Resolution: --- → DUPLICATE
Duplicate of bug: 1233259
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.
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.