Closed
Bug 1373220
Opened 8 years ago
Closed 8 years ago
Crash in nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild
Categories
(Core :: Widget: Win32, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla56
People
(Reporter: philipp, Assigned: handyman)
References
()
Details
(5 keywords, Whiteboard: [adv-main55+][adv-esr52.3+] tpi:+)
Crash Data
Attachments
(2 files)
2.09 MB,
video/mp4
|
Details | |
1.28 KB,
patch
|
jimm
:
review+
jcristau
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
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.
Comment 1•8 years ago
|
||
[Tracking Requested - why for this release]: Topcrash in early Beta 55 builds
tracking-firefox55:
--- → ?
Comment 2•8 years ago
|
||
Jim, are you the right person to start the investigation here since it's in Widget code?
Flags: needinfo?(jmathies)
Comment 3•8 years ago
|
||
Looks like it requires user interaction and involves chrome stuff.
Keywords: csectype-uaf,
sec-moderate
![]() |
||
Comment 4•8 years ago
|
||
(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:+
![]() |
||
Updated•8 years ago
|
Updated•8 years ago
|
Reporter | ||
Updated•8 years ago
|
Crash Signature: [@ nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild] → [@ nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild]
[@ RefPtr<T>::assign_with_AddRef | nsBaseWidget::AddChild]
Comment 5•8 years ago
|
||
Keep in mind that 55 is still being heavily throttled, so the absolute number of crashes may be a misleading indication of the severity :)
Comment 7•8 years ago
|
||
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
Comment 8•8 years ago
|
||
(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.
Comment 10•8 years ago
|
||
(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)
Reporter | ||
Comment 11•8 years ago
|
||
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
Updated•8 years ago
|
Flags: needinfo?(bugs)
Reporter | ||
Comment 12•8 years ago
|
||
str |
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
Reporter | ||
Comment 13•8 years ago
|
||
Are there any UAF crashes? Based on comments yes, but I saw only null+offset crashes with the stack traces similar to comment 0.
Reporter | ||
Comment 15•8 years ago
|
||
see https://crash-stats.mozilla.com/report/index/e36118a7-0621-4099-8524-718180170705 for an example of UAF
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)
Reporter | ||
Comment 17•8 years ago
|
||
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.
![]() |
||
Updated•8 years ago
|
Component: Untriaged → Widget: Win32
![]() |
||
Comment 22•8 years ago
|
||
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)
Assignee | ||
Comment 23•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee: nobody → davidp99
![]() |
||
Updated•8 years ago
|
Flags: needinfo?(jmathies)
![]() |
||
Comment 24•8 years ago
|
||
Looks like this was introduced in bug 530070.
![]() |
||
Updated•8 years ago
|
Attachment #8885994 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 25•8 years ago
|
||
sec-moderate... so it looks like we are good to go.
Keywords: checkin-needed
Comment 26•8 years ago
|
||
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.
Comment 27•8 years ago
|
||
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(davidp99)
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Comment 28•8 years ago
|
||
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?
Comment 29•8 years ago
|
||
(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 30•8 years ago
|
||
Comment on attachment 8885994 [details] [diff] [review]
widgetParent.patch
uaf in windows widget code, beta55+
Attachment #8885994 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 31•8 years ago
|
||
uplift |
Updated•8 years ago
|
Attachment #8885994 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
Flags: qe-verify+
Comment 32•8 years ago
|
||
uplift |
Keywords: checkin-needed
Comment 33•8 years ago
|
||
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+
Updated•8 years ago
|
Whiteboard: tpi:+ → [adv-main55+][adv-esr52.3+] tpi:+
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•