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)

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*
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.
Status: NEW → RESOLVED
Closed: 8 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: