Closed Bug 1373220 Opened 2 years ago Closed 2 years ago

Crash in nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild

Categories

(Core :: Widget: Win32, defect, P2, critical)

55 Branch
All
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox-esr52 55+ verified
firefox54 --- wontfix
firefox55 + verified
firefox56 + verified

People

(Reporter: philipp, Assigned: handyman)

References

()

Details

(5 keywords, Whiteboard: [adv-main55+][adv-esr52.3+] tpi:+)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-f164bedd-f815-46ff-b462-e930a0170615.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsCOMPtr_base::assign_with_AddRef(nsISupports*) 	xpcom/base/nsCOMPtr.cpp:44
1 	xul.dll 	nsBaseWidget::AddChild(nsIWidget*) 	widget/nsBaseWidget.cpp:580
2 	xul.dll 	nsWindow::SetParent(nsIWidget*) 	widget/windows/nsWindow.cpp:1261
3 	xul.dll 	nsViewManager::ReparentChildWidgets(nsView*, nsIWidget*) 	view/nsViewManager.cpp:829
4 	xul.dll 	nsViewManager::ReparentChildWidgets(nsView*, nsIWidget*) 	view/nsViewManager.cpp:842
5 	xul.dll 	nsViewManager::ReparentWidgets(nsView*, nsView*) 	view/nsViewManager.cpp:863
6 	xul.dll 	nsViewManager::InsertChild(nsView*, nsView*, nsView*, bool) 	view/nsViewManager.cpp:920
7 	xul.dll 	InsertViewsInReverseOrder 	layout/generic/nsSubDocumentFrame.cpp:1100
8 	xul.dll 	nsSubDocumentFrame::BeginSwapDocShells(nsIFrame*) 	layout/generic/nsSubDocumentFrame.cpp:1125
9 	xul.dll 	nsFrameLoader::SwapWithOtherLoader(nsFrameLoader*, nsIFrameLoaderOwner*, nsIFrameLoaderOwner*) 	dom/base/nsFrameLoader.cpp:1845
10 	xul.dll 	nsXULElement::SwapFrameLoaders(nsIFrameLoaderOwner*, mozilla::ErrorResult&) 	dom/xul/nsXULElement.cpp:1627
11 	xul.dll 	nsXULElement::SwapFrameLoaders(nsXULElement&, mozilla::ErrorResult&) 	dom/xul/nsXULElement.cpp:1607
12 	xul.dll 	mozilla::dom::XULElementBinding::swapFrameLoaders 	obj-firefox/dom/bindings/XULElementBinding.cpp:8090
13 	xul.dll 	mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp:2958
...

a number of user comments in those crash reports say they where trying to close a firebug window, like in
bp-2c91fdf3-e247-4c89-abfd-a8f9f0170520, bp-ed8efa01-9678-445d-9161-f61370170521 & bp-f164bedd-f815-46ff-b462-e930a0170615.
i wasn't able to reproduce this myself when playing around a little bit with the addon.

crashes with this signature when firebug is present (for others, see bug 1367686) seem to have started sometime in the 55.0a1 nightly cycle, the first instance was in build 20170514030207. this is the search i'm looking at for that:
https://crash-stats.mozilla.com/search/?signature=~nsBaseWidget%3A%3AAddChild&addons=~firebug%40software.joehewitt.com&release_channel=nightly&product=Firefox&date=%3E%3D2017-03-15T12%3A16%3A01.000Z&_facets=signature&_facets=version&_facets=user_comments&_facets=build_id&_facets=install_time&_facets=platform_pretty_version&_facets=useragent_locale&_facets=cpu_arch#facet-build_id

a portion of them is crashing in an address indicating a UAF situation.
[Tracking Requested - why for this release]: Topcrash in early Beta 55 builds
Jim, are you the right person to start the investigation here since it's in Widget code?
Flags: needinfo?(jmathies)
Looks like it requires user interaction and involves chrome stuff.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> [Tracking Requested - why for this release]: Topcrash in early Beta 55 builds

This doesn't look like a 55 top crasher to me. It's a pretty bad crash though, showing up in 53 currently. Doesn't appear in 54 yet.

Definitely need to fix this.
Flags: needinfo?(jmathies)
Priority: -- → P2
Whiteboard: tpi:+
Crash Signature: [@ nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild] → [@ nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild] [@ RefPtr<T>::assign_with_AddRef | nsBaseWidget::AddChild]
Keep in mind that 55 is still being heavily throttled, so the absolute number of crashes may be a misleading indication of the severity :)
Jim, can you help prioritize this?
Flags: needinfo?(jmathies)
The Firebug correlation is a red herring. Only a third of the crashes with this signature have firebug installed, and those are largely beta users (as you might expect). Since processing release crashes is throttled to 10% the actual fraction of crashing users with firebug installed is even lower.

A bunch of the UAF stacks I looked at were under nsPluginInstanceOwner::CreateWidget() (about a quarter of them). Another third were under nsViewManager::InsertChild.

I don't know that this always requires user interaction. A lot of the ones with comments mention that (e.g. comment 0, or moving their twitch.tv streams to another window), but a bunch don't. sec-moderate might be too conservative.
Group: core-security → core-security-release
Keywords: testcase-wanted
Summary: Crash in nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild with Firebug → Crash in nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild
(In reply to Daniel Veditz [:dveditz] from comment #7)
> The Firebug correlation is a red herring. Only a third of the crashes with
> this signature have firebug installed, and those are largely beta users (as
> you might expect). Since processing release crashes is throttled to 10% the
> actual fraction of crashing users with firebug installed is even lower.
> 
> A bunch of the UAF stacks I looked at were under
> nsPluginInstanceOwner::CreateWidget() (about a quarter of them). Another
> third were under nsViewManager::InsertChild.
> 
> I don't know that this always requires user interaction. A lot of the ones
> with comments mention that (e.g. comment 0, or moving their twitch.tv
> streams to another window), but a bunch don't. sec-moderate might be too
> conservative.

Maybe we should reconsider this being a P2 given this comment.
Andrew, what do you think (re: comment 8)?
Flags: needinfo?(continuation)
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #9)
> Andrew, what do you think (re: comment 8)?

I don't have any opinion on priority flags.
Flags: needinfo?(continuation)
i found a way to reproduce the crash with firebug and when regression-testing the issue it appears that bug 1358832 introduced the crash with the addon.
Blocks: 1358832
Flags: needinfo?(bugs)
str in a new profile: (e10s has to be off)
- install firebug
- go to a website
- click on the firebug icon
- click on the button to detach the firebug panel to a new window
- click on the down arrow next to script to enable that panel
- click on the reload button on the website
- click on the script tab in the firebug window to focus it again
- go to the windows task bar and close the firebug window
- firefox crashes
Attached video str.mp4
Are there any UAF crashes? Based on comments yes, but I saw only null+offset crashes with the stack traces similar to comment 0.
Hmm, I can't seem to play that mp4 on linux and I don't understand comment 12.
"click on the down arrow next to script to enable that panel" ?
I don't have "script" tab in firebug window.
Flags: needinfo?(bugs)
the STR only work when e10s is switched off (browser.tabs.remote.autostart.2 = false), otherwise firebug is just a skin for the built-in devtools). the video should work with alternative players like vlc...
oh, e10s off.

By any chance, could someone who can reproduce this try to make the following variables strong
http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/view/nsViewManager.cpp#823-824
So, use nsCOMPtr<nsIWidget> there, not nsIWidget*
but let me retry
Still no luck.
Duplicate of this bug: 1367686
Component: Untriaged → Widget: Win32
David, could you please take a look at this and the data in dupe bug 1367686 and see if you can track down a fix.
Flags: needinfo?(davidp99)
Patch is pretty straightforward.  GetParent() is giving the new parent when we want the old one.

-----

The STR in comment 12 is very finicky (kudos to the OP for finding it).  Slight deviations negate it and there are a lot of steps.  The main things that tripped me up were:

* You only restart the browser to turn off e10s.  Specifically, don't restart after installing Firebug.
* There currently is no "down arrow next to [the script tab]" but you just need to select the tab.  (An arrow then appears... you dont need to push that)
* After reloading the page, the script tab will still be selected.  The idea there is just to refocus the window.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90b593e0d1ae02250c55c53297db0ea085f54165
Flags: needinfo?(davidp99)
Attachment #8885994 - Flags: review?(jmathies)
Assignee: nobody → davidp99
Flags: needinfo?(jmathies)
Looks like this was introduced in bug 530070.
Attachment #8885994 - Flags: review?(jmathies) → review+
sec-moderate... so it looks like we are good to go.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb0fcbcdd504201af4caa94ad9a7d8383b43926

I've confirmed that this grafts cleanly to Beta and ESR52. Please request approval on the patch when you're comfortable doing so.
https://hg.mozilla.org/mozilla-central/rev/6cb0fcbcdd50
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(davidp99)
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8885994 [details] [diff] [review]
widgetParent.patch

Here we go --

(ESR52)
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Fairly high volume Windows crash
User impact if declined: 
Crashes
Fix Landed on Version:
56
Risk to taking this patch (and alternatives if risky): 
Low.  This couldn't introduce an issue worse than the UAF it fixes.
String or UUID changes made by this patch: 
None

--------

(Beta)
Approval Request Comment
[Feature/Bug causing the regression]:
Windows widget, specifically window parentage
[User impact if declined]:
Crashes from normal usage
[Is this code covered by automated tests?]:
No
[Has the fix been verified in Nightly?]:
Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
I think yes.  STR is in comment 12, with some notes on STR gotchas in comment 23.
[List of other uplifts needed for the feature/fix]:
None
[Is the change risky?]:
No
[Why is the change risky/not risky?]:
This couldn't introduce an issue worse than the UAF it fixes.
[String changes made/needed]:
None
Flags: needinfo?(davidp99)
Attachment #8885994 - Flags: approval-mozilla-esr52?
Attachment #8885994 - Flags: approval-mozilla-beta?
(In reply to Jim Mathies [:jimm] from comment #24)
> Looks like this was introduced in bug 530070.

Wow, much older than I would have thought given the crash volume spike when 53 was released. Maybe something else changed and happened to start using the buggy function in just the right way?
Comment on attachment 8885994 [details] [diff] [review]
widgetParent.patch

uaf in windows widget code, beta55+
Attachment #8885994 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8885994 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Flags: qe-verify+
Reproduced the crash on an affected build (55.0b10, 20170717063821, e26b1f5d635e) [1] using the str from Comment 12 on Windows 10 x64.

I can confirm these str are no longer crashing on Windows 10 x64 and Windows 7 x64 using:
    - 52.2esr (20170723091719, 3856be3d222f)
    - 55.0b11 (20170720101430, e5f14b9ae6c4)
    - 56.0a1 (2017-07-23, 20170723030206)    

[1] bp-ef4173ac-4784-4d79-97b7-6aea30170724
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: tpi:+ → [adv-main55+][adv-esr52.3+] tpi:+
Group: core-security-release
Duplicate of this bug: 1359390
You need to log in before you can comment on or make changes to this bug.