Closed Bug 1515113 Opened 6 years ago Closed 6 years ago

use-after-free in [@ nsIPresShell::GetRootScrollFrame]

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Platform:
Unspecified
Android
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 + fixed
firefox66 + fixed

People

(Reporter: tsmith, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [post-critsmash-triage])

Attachments

(4 files, 1 obsolete file)

testcase.html
439 bytes, text/html
Details
prefs.js
14.19 KB, application/x-javascript
Details
patch
8.32 KB, patch
kats
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
beta patch
7.87 KB, patch
RyanVM
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file testcase.html —
I don't have access to an ASan build on android yet so this will have to do. Crash|SIGSEGV|0x610072|12 0|libxul.so|nsIPresShell::GetRootScrollFrame() const|hg:hg.mozilla.org/mozilla-central:layout/base/nsFrameManager.h:8f91fb1acea5ed5585a8502599e51adfc1618de2|47|0x0 1|libxul.so|nsIPresShell::GetRootScrollFrameAsScrollable() const|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2423|0xb 2|libxul.so|MobileViewportManager::ShrinkToDisplaySizeIfNeeded(nsViewportInfo&, mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&)|hg:hg.mozilla.org/mozilla-central:layout/base/MobileViewportManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|512|0xb 3|libxul.so|MobileViewportManager::RefreshViewportSize(bool)|hg:hg.mozilla.org/mozilla-central:layout/base/MobileViewportManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|497|0x16 4|libxul.so|MobileViewportManager::RequestReflow()|hg:hg.mozilla.org/mozilla-central:layout/base/MobileViewportManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|122|0xd 5|libxul.so|mozilla::PresShell::ResizeReflow(int, int, int, int, nsIPresShell::ResizeReflowOptions)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1866|0x8 6|libxul.so|nsViewManager::DoSetWindowDimensions(int, int)|hg:hg.mozilla.org/mozilla-central:view/nsViewManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|182|0x17 7|libxul.so|nsDocumentViewer::SetBoundsWithFlags(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, unsigned int)|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2042|0x14 8|libxul.so|nsDocumentViewer::SetBounds(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2065|0x10 9|libxul.so|nsDocumentViewer::SetBoundsWithFlags(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, unsigned int)|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2057|0xf 10|libxul.so|nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int)|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|5171|0xe 11|libxul.so|non-virtual thunk to nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int)|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|0|0x5 12|libxul.so|nsFrameLoader::UpdateBaseWindowPositionAndSize(nsSubDocumentFrame*)|hg:hg.mozilla.org/mozilla-central:dom/base/nsFrameLoader.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2321|0x29 13|libxul.so|nsFrameLoader::UpdatePositionAndSize(nsSubDocumentFrame*)|hg:hg.mozilla.org/mozilla-central:dom/base/nsFrameLoader.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2295|0xa 14|libxul.so|nsSubDocumentFrame::ReflowFinished()|hg:hg.mozilla.org/mozilla-central:layout/generic/nsSubDocumentFrame.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|818|0xc 15|libxul.so|non-virtual thunk to nsSubDocumentFrame::ReflowFinished()|hg:hg.mozilla.org/mozilla-central:layout/generic/nsSubDocumentFrame.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|0|0x5 16|libxul.so|mozilla::PresShell::HandlePostedReflowCallbacks(bool)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|3885|0x8 17|libxul.so|mozilla::PresShell::DidDoReflow(bool)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|8350|0xf 18|libxul.so|mozilla::PresShell::ResizeReflowIgnoreOverride(int, int, int, int, nsIPresShell::ResizeReflowOptions)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1995|0x10 19|libxul.so|nsViewManager::DoSetWindowDimensions(int, int)|hg:hg.mozilla.org/mozilla-central:view/nsViewManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|182|0x17 20|libxul.so|nsDocumentViewer::SetBoundsWithFlags(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, unsigned int)|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2042|0x14 21|libxul.so|nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int)|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|5171|0xe 22|libxul.so|non-virtual thunk to nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int)|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|0|0x5 23|libxul.so|nsWebShellWindow::WindowResized(nsIWidget*, int, int)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsWebShellWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|290|0x11 24|libxul.so|nsWebShellWindow::WidgetListenerDelegate::WindowResized(nsIWidget*, int, int)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsWebShellWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|740|0x16 25|libxul.so|nsWindow::OnSizeChanged(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&)|hg:hg.mozilla.org/mozilla-central:widget/android/nsWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1817|0xc 26|libxul.so|nsWindow::Resize(double, double, double, double, bool)|hg:hg.mozilla.org/mozilla-central:widget/android/nsWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1620|0xc 27|libxul.so|nsWindow::Resize(double, double, bool)|hg:hg.mozilla.org/mozilla-central:widget/android/nsWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1604|0x28 28|libxul.so|nsXULWindow::SetSize(int, int, bool)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsXULWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|580|0x1c 29|libxul.so|nsContentTreeOwner::SetSize(int, int, bool)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsContentTreeOwner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|512|0x11 30|libxul.so|non-virtual thunk to nsContentTreeOwner::SetSize(int, int, bool)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsContentTreeOwner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|0|0x5 31|libxul.so|nsGlobalWindowOuter::ResizeByOuter(int, int, mozilla::dom::CallerType, mozilla::ErrorResult&)|hg:hg.mozilla.org/mozilla-central:dom/base/nsGlobalWindowOuter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|4905|0x18 32|libxul.so|nsGlobalWindowInner::ResizeBy(int, int, mozilla::dom::CallerType, mozilla::ErrorResult&)|hg:hg.mozilla.org/mozilla-central:dom/base/nsGlobalWindowInner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|3480|0x13 33|libxul.so|mozilla::dom::Window_Binding::resizeBy(JSContext*, JS::Handle<JSObject*>, nsGlobalWindowInner*, JSJitMethodCallArgs const&)|s3:gecko-generated-sources:609c24cdbf77eb87c64e5f2116444d78efa5fe9e90b3d5ae351d3b3934a74fa3ac1b3a0c4ab6ed86a1d829abac8bfe58e28e82dc186b3a16ac48868a75285da8/dom/bindings/WindowBinding.cpp:|3919|0x11 34|libxul.so|bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)|hg:hg.mozilla.org/mozilla-central:dom/bindings/BindingUtils.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|3064|0xd 35|libxul.so|CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|443|0xe 36|libxul.so|js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|535|0xb 37|libxul.so|InternalCall(JSContext*, js::AnyInvokeArgs const&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|590|0x14 38|libxul.so|Interpret(JSContext*, js::RunState&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|594|0x8 39|libxul.so|js::RunScript(JSContext*, js::RunState&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|423|0x9 40|libxul.so|js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|563|0xd 41|libxul.so|InternalCall(JSContext*, js::AnyInvokeArgs const&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|590|0x14 42|libxul.so|<name omitted>|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|606|0x7 43|libxul.so|JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>)|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2649|0x2a 44|libxul.so|mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&)|s3:gecko-generated-sources:344e7bbc1d2eb535f4d85b157b525fa18eb1e4688bb70bf6e3c51b02fe597784d3fe3e6a2b078d2e5b5c857d2435e216ad8a578cae88f140c6e1c2a969833ac5/dom/bindings/EventHandlerBinding.cpp:|265|0x24 45|libxul.so|void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*)|s3:gecko-generated-sources:3760fb8938acb537e28af191191b3b28eae59c21a2b0dcc49f0990b2613d2b29a3f705864d9a9ad5a160e5faa013e2d0b447215b3fc0b25795d7ebbd0dc4ce29/dist/include/mozilla/dom/EventHandlerBinding.h:|363|0x1e 46|libxul.so|mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*)|hg:hg.mozilla.org/mozilla-central:dom/events/JSEventHandler.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|205|0x36 47|libxul.so|mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*)|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1044|0xc 48|libxul.so|mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool)|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1239|0x1d 49|libxul.so|mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&)|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|346|0x18 50|libxul.so|mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|548|0x18 51|libxul.so|mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1038|0x23 52|libxul.so|(anonymous namespace)::AsyncTimeEventRunner::Run()|hg:hg.mozilla.org/mozilla-central:dom/smil/nsSMILTimedElement.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|96|0x19 53|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1157|0x8 54|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|468|0x11 55|libxul.so|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|88|0x10 56|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:8f91fb1acea5ed5585a8502599e51adfc1618de2|314|0xb 57|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:8f91fb1acea5ed5585a8502599e51adfc1618de2|307|0xb 58|libxul.so|nsBaseAppShell::Run()|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|137|0x9 59|libxul.so|nsAppStartup::Run()|hg:hg.mozilla.org/mozilla-central:toolkit/components/startup/nsAppStartup.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|271|0x9 60|libxul.so|XREMain::XRE_mainRun()|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|4616|0x8 61|libxul.so|XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|4754|0x8 62|libxul.so|XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|4839|0xf 63|libxul.so|GeckoStart|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAndroidStartup.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|47|0xd 64|libxul.so|mozilla::BootstrapImpl::GeckoStart(_JNIEnv*, char**, int, mozilla::StaticXREAppData const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/Bootstrap.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|71|0x11 65|libmozglue.so|Java_org_mozilla_gecko_mozglue_GeckoLoader_nativeRun|hg:hg.mozilla.org/mozilla-central:mozglue/android/APKOpen.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|371|0x18 66|base.odex||||0x8eeab7 67|||||0xed4ec0e4 68|libart.so||||0x735b84 69|||||0xffffffff 70|dalvik-main space (deleted)||||0x1422a0 71|libart.so||||0x313fc3 72|dalvik-LinearAlloc (deleted)||||0x98b8
Flags: in-testsuite?
+ Maire while Sean is out.
The attached test case hits the following assertion but unreduced (and other) versions trigger the above crash at different addresses. Assertion failure: get() (dereferencing a UniquePtr containing nullptr), at src/obj-firefox/dist/include/mozilla/UniquePtr.h:302
Hi Cameron -- I'm hoping to get this fixed soon. Thoughts about what's going on here?
Flags: needinfo?(cam)
The testcase didn't seem to crash my Fennec Beta (65). Possible regression?
status-firefox65: --- → ?
status-firefox66: --- → affected
status-geckoview65: affected → ---
Severity: normal → critical
Tyson, does the testcase require some non-default prefs? (we don't allow window.resizeBy by default, do we?)
Flags: needinfo?(twsmith)
Attached file prefs.js —
Here is the prefs.js file that was used. FWIW I've only been able to reproduce this with Fennec.
Flags: needinfo?(twsmith)
How do you install a prefs.js file into a Fennec installation? I opened the testcase with just the "dom.disable_window_move_resize" pref changed manually (which seems to be the relevant one based on a cursory look), but did not reproduce the crash.
Tyson, do you know the answer to Botond's question?
Flags: needinfo?(twsmith)
Given that this requires non-default prefs, tentatively setting to P3, unless anyone thinks otherwise.
Priority: -- → P3
I've verified I can reproduce this with default prefs on the latest available m-c debug build. BuildID=20190102094850 SourceStamp=5826b2352ac08248205d3b0e29587ab8ad415bfe I put testcase.html on a web server and opened it on the device. It can take up to a minute to repro but opening it in multiple tabs speeds up the process.
Flags: needinfo?(twsmith)
I was able to reproduce exactly once despite many many efforts. https://crash-stats.mozilla.org/report/index/6beb5827-d979-4b79-830f-2bb360190103 Looks like mFrameConstructor is null on the presshell (based on the stack and comment 2). Which doesn't make a lot of sense, it is cleared in ~PresShell, and created (as a UniquePtr) in PresShell::Init, which gets called pretty much immediately after we create PresShells at the one location we create PresShells. The AccessibleCaret code observes Reflow done notifications, and the first thing is does is FlushLayout before updating the caret. Which is silly. I created a try build here with printf_stderrs if anyone that can reproduce wants to capture logcat https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f482b6c60161a8594aec9bf0cbcc9baa3df83e6
Flags: needinfo?(twsmith)

Thanks to Tyson for running some try builds I was able to figure this out. The ResizeReflowIgnoreOverride call in MobileViewportManager::RefreshViewportSize kills the presshell and MobileViewportManager, so the call into the presshell in ShrinkToDisplaySizeIfNeeded is using deleted memory for mPresShell pointer.

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

The AccessibleCaret code observes Reflow done notifications, and the first
thing is does is FlushLayout before updating the caret. Which is silly.

Looks like that is covered by bug 1445794, but reflow could still potentially kill us via reflow callbacks that ask for a flush.

Flags: needinfo?(twsmith)
Flags: needinfo?(cam)
Attached patch patch — — Splinter Review
Assignee: nobody → tnikkel
Attachment #9034806 - Flags: review?(kats)
Keywords: csectype-uaf
Summary: crash in [@ nsIPresShell::GetRootScrollFrame] → use-after-free in [@ nsIPresShell::GetRootScrollFrame]

Nice!

Priority: P3 → P2

Comment on attachment 9034806 [details] [diff] [review]
patch

Review of attachment 9034806 [details] [diff] [review]:

LGTM, thanks!

::: layout/base/MobileViewportManager.cpp
@@ +523,5 @@
>    // Update internal state.
>    mMobileViewportSize = viewport;
>  
> +  // ResizeReflowIgnoreOverride can kill us.
> +  RefPtr<MobileViewportManager> strongThis(this);

you don't like the kungFuDeathGrip? :p

Attachment #9034806 - Flags: review?(kats) → review+

Comment on attachment 9034806 [details] [diff] [review]
patch

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: I'll strip the commit msg and comment from the patch but not much we can do to hide this, the fix makes it obvious which object dies and what call causes that, they'd need a fuzzer or some smarts/work to trigger it though

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: probably all of them

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: should be pretty trivial

How likely is this patch to cause regressions; how much testing does it need?: very unlikely

Attachment #9034806 - Flags: sec-approval?

(In reply to Timothy Nikkel (:tnikkel) from comment #12)

The ResizeReflowIgnoreOverride call in MobileViewportManager::RefreshViewportSize kills the presshell and MobileViewportManager, so the call into the presshell in ShrinkToDisplaySizeIfNeeded is using deleted memory for mPresShell pointer.

Oh whoops. My bad for not catching that during review.

(In reply to Timothy Nikkel (:tnikkel) from comment #16)

Which older supported branches are affected by this flaw?: probably all of them

If not all supported branches, which bug introduced the flaw?: None

The problematic call to ShrinkToDisplaySizeIfNeeded() was introduced in bug 1423709, which landed in 65, so branches older than 65 should not be affected.

Blocks: 1423709

sec-approval+ for trunk. Let's get a beta nomination on a patch so we can avoid shipping the issue.

Keywords: sec-critical
Attachment #9034806 - Flags: sec-approval? → sec-approval+
Attached patch beta patch (obsolete) — — Splinter Review

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1423709

User impact if declined: sec uaf

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): just holds a strong ref to this to keep it alive

String changes made/needed: none

Attachment #9035125 - Flags: approval-mozilla-beta?
Attached patch beta patch — — Splinter Review
Attachment #9035125 - Attachment is obsolete: true
Attachment #9035125 - Flags: approval-mozilla-beta?

Comment on attachment 9035131 [details] [diff] [review]
beta patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1423709

User impact if declined: sec uaf

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): just holds a strong ref to this to keep it alive

String changes made/needed: none

Attachment #9035131 - Flags: approval-mozilla-beta?

https://hg.mozilla.org/mozilla-central/rev/1913de91fd89
https://hg.mozilla.org/mozilla-central/rev/8d9995ade756

Group: layout-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Comment on attachment 9035131 [details] [diff] [review]
beta patch

[Triage Comment]
Fixes a sec-crit UAF. Approved for 65.0b10.

Attachment #9035131 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
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: