Closed Bug 1484559 Opened 2 years ago Closed 2 years ago

use-after-poison in [@ RefreshDriver]

Categories

(Core :: Layout, defect)

Desktop
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 63+ verified
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 + verified
firefox64 --- verified

People

(Reporter: tsmith, Assigned: mats)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main63-][adv-esr60.3+])

Attachments

(6 files)

Attached file testcase.html
==5887==ERROR: AddressSanitizer: use-after-poison on address 0x6250002b2c60 at pc 0x7f71927bd7d0 bp 0x7ffdb0f36440 sp 0x7ffdb0f36438
READ of size 8 at 0x6250002b2c60 thread T0
    #0 0x7f71927bd7cf in RefreshDriver src/layout/generic/nsGfxScrollFrame.cpp:1865:21
    #1 0x7f71927bd7cf in RemoveObserver src/layout/generic/nsGfxScrollFrame.cpp:1875
    #2 0x7f71927bd7cf in ~AsyncSmoothMSDScroll src/layout/generic/nsGfxScrollFrame.cpp:1859
    #3 0x7f71927bd7cf in mozilla::ScrollFrameHelper::AsyncSmoothMSDScroll::Release() src/layout/generic/nsGfxScrollFrame.cpp:1774
    #4 0x7f71921a147c in Release src/obj-firefox/dist/include/mozilla/RefPtr.h:42:11
    #5 0x7f71921a147c in Release src/obj-firefox/dist/include/mozilla/RefPtr.h:407
    #6 0x7f71921a147c in ~RefPtr src/obj-firefox/dist/include/mozilla/RefPtr.h:80
    #7 0x7f71921a147c in nsRefreshDriver::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1877
    #8 0x7f71921b4ba2 in TickDriver src/layout/base/nsRefreshDriver.cpp:324:13
    #9 0x7f71921b4ba2 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:299
    #10 0x7f71921b46c1 in mozilla::RefreshDriverTimer::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:316:5
    #11 0x7f71921b7cb1 in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:755:5
    #12 0x7f71921b7cb1 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:671
    #13 0x7f71921b1b99 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:512:20
    #14 0x7f7187edd240 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1235:14
    #15 0x7f7187ee5fa5 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #16 0x7f71890c3244 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:125:5
    #17 0x7f7188fc50cc in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #18 0x7f7188fc50cc in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #19 0x7f7188fc50cc in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #20 0x7f7191adfaf6 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #21 0x7f7195b8ef7b in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290:30
    #22 0x7f7195e47e7c in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4799:22
    #23 0x7f7195e4af9d in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4944:8
    #24 0x7f7195e4c74e in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5036:21
    #25 0x4f591c in do_main src/browser/app/nsBrowserApp.cpp:233:22
    #26 0x4f591c in main src/browser/app/nsBrowserApp.cpp:311
    #27 0x7f71ab52d82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #28 0x424edc in _start (firefox+0x424edc)

0x6250002b2c60 is located 2912 bytes inside of 8192-byte region [0x6250002b2100,0x6250002b4100)
allocated by thread T0 here:
    #0 0x4c5623 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f7187e70733 in AllocateChunk src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:193:15
    #2 0x7f7187e70733 in InternalAllocate src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:228
    #3 0x7f7187e70733 in Allocate src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:75
    #4 0x7f7187e70733 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:80
    #5 0x7f71924b183a in AllocateByFrameID src/layout/base/nsPresArena.h:39:12
    #6 0x7f71924b183a in AllocateFrame src/layout/base/nsIPresShell.h:206
    #7 0x7f71924b183a in operator new src/layout/generic/ViewportFrame.cpp:34
    #8 0x7f71924b183a in NS_NewViewportFrame(nsIPresShell*, mozilla::ComputedStyle*) src/layout/generic/ViewportFrame.cpp:31
    #9 0x7f71922df18e in nsCSSFrameConstructor::ConstructRootFrame() src/layout/base/nsCSSFrameConstructor.cpp:2661:5
    #10 0x7f719220f381 in mozilla::PresShell::Initialize() src/layout/base/PresShell.cpp:1799:36
    #11 0x7f718be4dd87 in nsContentSink::StartLayout(bool) src/dom/base/nsContentSink.cpp:1274:26
    #12 0x7f718a862a92 in nsHtml5TreeOpExecutor::StartLayout(bool*) src/parser/html/nsHtml5TreeOpExecutor.cpp:673:18
    #13 0x7f718a85dede in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*, bool*) src/parser/html/nsHtml5TreeOperation.cpp:1204:17
    #14 0x7f718a85ae36 in nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:489:17
    #15 0x7f718a8672db in nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:121:18
    #16 0x7f7187edd240 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1235:14
    #17 0x7f7187ee5fa5 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #18 0x7f71890c325e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #19 0x7f7188fc50cc in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #20 0x7f7188fc50cc in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #21 0x7f7188fc50cc in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #22 0x7f7191adfaf6 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #23 0x7f7195b8ef7b in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290:30
    #24 0x7f7195e47e7c in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4799:22
    #25 0x7f7195e4af9d in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4944:8
    #26 0x7f7195e4c74e in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5036:21
    #27 0x4f591c in do_main src/browser/app/nsBrowserApp.cpp:233:22
    #28 0x4f591c in main src/browser/app/nsBrowserApp.cpp:311
Flags: in-testsuite?
Attached file prefs.js
required to repro
Should be protected by frame-poisoning:

#2  0x00007f8caa936629 in mozilla::ScrollFrameHelper::AsyncSmoothMSDScroll::RefreshDriver (this=0x611000b08500, aCallee=0x625000d7abd0) at layout/generic/nsGfxScrollFrame.cpp:1816
1816        return aCallee->mOuter->PresContext()->RefreshDriver();
(rr) p aCallee->mOuter
$1 = (nsContainerFrame *) 0x7ffffffff0dea7ff
Severity: normal → critical
Assignee: nobody → mats
These prefs are necessary to crash when loading the attached testcase:
user_pref("layout.accessiblecaret.enabled", true);
user_pref("browser.tabs.remote.autostart", false);
user_pref("browser.tabs.remote.autostart.1", false);
user_pref("browser.tabs.remote.autostart.2", false);
Attached patch fixSplinter Review
The problem is that the current RemoveObserver() code assumes
that the scroll frame outlives its mAsyncScroll / mAsyncSmoothMSDScroll
instances, i.e. that those strong refs are the last (only?) ones.
This is a false assumption since the RefreshDriver holds a local
strong ref on the stack while doing the observer callback.
The testcase destroys the frame in that callback so the observer
dtor runs after the frame is already destroyed.  When the local
instance goes out of scope its dtor call RemoveObserver()
which uses the destroyed frame. (see stacks above)
Attachment #9002599 - Flags: review?(dholbert)
BTW, I deliberately left out the details above from the patch
since I'm not sure this crash is always mitigated by frame
poisoning or if there's a way these callback can also destroy
the shell in which case we would have a regular use-after-free.
I don't think so, but just in case.
Comment on attachment 9002599 [details] [diff] [review]
fix

Review of attachment 9002599 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #9002599 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8d234a983ae65eee63239a8536dd24428f6db1
https://hg.mozilla.org/mozilla-central/rev/6d8d234a983a
Group: layout-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Per IRC discussion with Mats, this can ride the trains.
Whiteboard: [post-critsmash-triage]
Comment on attachment 9002599 [details] [diff] [review]
fix

Needed for bug 1490561 - see uplift request there.
Attachment #9002599 - Flags: approval-mozilla-geckoview62?
Attachment #9002599 - Flags: approval-mozilla-esr60?
Attachment #9002599 - Flags: approval-mozilla-beta?
Mats, the patch in comment #9 landed during the 63 nightly cycle so it should be in 63 beta now but you requested an uplift to the beta branch, is there something else in this bug to uplift to 63 beta? Thanks
Flags: needinfo?(mats)
Oh OK, only the other branches then :-)
Flags: needinfo?(mats)
Attachment #9002599 - Flags: approval-mozilla-beta?
Comment on attachment 9002599 [details] [diff] [review]
fix

Per bug 1490561 c19.
Attachment #9002599 - Flags: approval-mozilla-geckoview62? → approval-mozilla-geckoview62-
Comment on attachment 9002599 [details] [diff] [review]
fix

This doesn't graft cleanly to ESR60. Please attach a rebased patch and re-request approval.
Flags: needinfo?(mats)
Attachment #9002599 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60-
Attached patch esr60 patchSplinter Review
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:
Flags: needinfo?(mats)
Attachment #9014568 - Flags: approval-mozilla-esr60?
Comment on attachment 9014568 [details] [diff] [review]
esr60 patch

Needed for bug 1490561, approved for ESR 60.3.
Attachment #9014568 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
I have attempted to reproduce this issue in order to verify it in Nightly, but I could not.

After loading the prefs mentioned in comment 1, the following error would be displayed in Nightly:
"The proxy server is refusing connections" and the steps could not be continued.

If I set the prefs mentioned in comment 5, and load test case from comment 0, the crash would not reproduce.

What am I missing? I have attempted to reproduce the crash in Nightly v63.0a1 from 2018-08-15.
Flags: needinfo?(twsmith)
Flags: needinfo?(mats)
Is it OS specific?
(In reply to Bodea Daniel [:danibodea] from comment #19)
> If I set the prefs mentioned in comment 5, and load test case from comment
> 0, the crash would not reproduce.

Right, the only prefs you need are the ones in comment 5.

> What am I missing? I have attempted to reproduce the crash in Nightly
> v63.0a1 from 2018-08-15.

The crash will likely only reproduce in an ASAN Linux build.
It might take a few minutes for the crash to occur (one of
the testacases for this bug and bug 1490561 did anyway -
I don't recall which one).
Flags: needinfo?(mats)
Flags: needinfo?(twsmith)
I have tried reproducing it again on Ubuntu 16 and Ubuntu 18 and these were my steps:
1. Downloaded a asan-reporter build from a date prior to the date of the push in mozilla central:
http://archive.mozilla.org/pub/firefox/nightly/2018/08/2018-08-20-10-01-29-mozilla-central/firefox-63.0a1.en-US.linux-x86_64-asan-reporter.tar.bz2
2. Opened browser with a profile that has update disabled.
3. Went to "about:config" and set prefs:
user_pref("layout.accessiblecaret.enabled", true);
user_pref("browser.tabs.remote.autostart", false);
4. The other 2 required prefs were not in the pref list, so I created them: 
user_pref("browser.tabs.remote.autostart.1", false);
user_pref("browser.tabs.remote.autostart.2", false);
*Is this a correct step? Is it known that these two prefs aren't there?*
5. Opened the attached test case from comment 0:
https://bugzilla.mozilla.org/attachment.cgi?id=9002323

Actual: The page kept refreshing continuously for more than 20 minutes, but no crash occurred.

@Mats: I cannot verify this issue until I can reproduce it. Do you have any idea of any other details that I might be missing from the reproduction process? 

Thank you!
Flags: needinfo?(mats)
OS: Unspecified → Linux
Hardware: Unspecified → Desktop
I think you need to restart the browser after entering those prefs,
or alternatively just add them directly in the "prefs.js" file in
the profile directory before starting.  So if you did that, then
your steps seem correct.

It isn't super-important to verify this fix though, so don't worry
about it if the crash doesn't occur for you.
Flags: needinfo?(mats)
I have managed to reproduce the issue in Nightly asan-reporter build from 2018-08-19 using the STR from comment 22 and Mats' suggestion from comment 23 (you were right!) and I have verified the fix in Nightly v63.0a1 from 2018-08-31.
The crash did not occur.

Furthermore, I would need an asan-reporter build for Beta and ESR60 verification, wouldn't I?

Could I also verify it for those versions? Also, considering Mats' comment 23, should I?

Thanks!
The fix is now verified in Beta v63.0b13, asan build containing the fix, from taskcluster:
https://index.taskcluster.net/v1/task/gecko.v2.mozilla-beta.latest.firefox.linux64-asan-opt/artifacts/public/build/target.tar.bz2
No crash occurred.

The fix is also verified in ERS60 v60.2.3esr, asan build containing the fix, from taskcluster:
https://index.taskcluster.net/v1/task/gecko.v2.mozilla-esr60.latest.firefox.linux64-asan-opt/artifacts/public/build/target.tar.bz2
No crash occurred either.

Uplift Successful! Thank you!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63-]
Whiteboard: [post-critsmash-triage][adv-main63-] → [post-critsmash-triage][adv-main63-][adv-esr60.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.