Closed
Bug 874486
Opened 11 years ago
Closed 11 years ago
ASAN: Crashtest layout/xul/tree/crashtests/409807-1.xul triggers error
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
firefox21 | --- | unaffected |
firefox22 | --- | unaffected |
firefox23 | + | fixed |
firefox24 | --- | fixed |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: decoder, Assigned: spohl)
References
(Blocks 1 open bug)
Details
(5 keywords, Whiteboard: [asan][asan-test-failure][lion-scrollbars+][adv-main23-])
Attachments
(1 file)
1.60 KB,
patch
|
roc
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This crashtest in content/xul/templates/src/crashtests/329335-1.xul fails with AddressSanitizer on mozilla-central revision 81dd97739fa1. This failure seems to reproduce well in debug and opt builds. Here is the ASan error trace from a debug build: ==6207== ERROR: AddressSanitizer: use-after-poison on address 0x2b82d90df978 at pc 0x2b82c0461c3f bp 0x7fff000b2270 sp 0x7fff000b2268 READ of size 4 at 0x2b82d90df978 thread T0 #0 0x2b82c0461c3e in nsTreeBodyFrame::UpdateScrollbars(nsTreeBodyFrame::ScrollParts const&) layout/xul/tree/nsTreeBodyFrame.cpp:864 #1 0x2b82c045e67b in nsTreeBodyFrame::FullScrollbarsUpdate(bool) layout/xul/tree/nsTreeBodyFrame.cpp:4623 #2 0x2b82c045d2b3 in nsTreeBodyFrame::SetView(nsITreeView*) layout/xul/tree/nsTreeBodyFrame.cpp:520 #3 0x2b82c047b21d in nsTreeBoxObject::GetView(nsITreeView**) layout/xul/tree/nsTreeBoxObject.cpp:154 #4 0x2b82c045b124 in nsTreeBodyFrame::EnsureView() layout/xul/tree/nsTreeBodyFrame.cpp:363 #5 0x2b82c045de49 in nsTreeBodyFrame::ReflowFinished() layout/xul/tree/nsTreeBodyFrame.cpp:409 #6 0x2b82c045e7bc in non-virtual thunk to nsTreeBodyFrame::ReflowFinished() layout/xul/tree/nsTreeBodyFrame.cpp:448 #7 0x2b82bf24fe0b in PresShell::HandlePostedReflowCallbacks(bool) layout/base/nsPresShell.cpp:3697 #8 0x2b82bf2468c2 in PresShell::DidDoReflow(bool) layout/base/nsPresShell.cpp:7634 #9 0x2b82bf25120e in PresShell::ProcessReflowCommands(bool) layout/base/nsPresShell.cpp:7936 #10 0x2b82bf250a69 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:3891 #11 0x2b82bf279841 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:959 #12 0x2b82bf27df0d in mozilla::RefreshDriverTimer::Tick() layout/base/nsRefreshDriver.cpp:158 #13 0x2b82c1b0e28d in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:547 #14 0x2b82c1b0eac5 in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:634 #15 0x2b82c1b0313a in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:627 #16 0x2b82c1a503d1 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan64dbg/xpcom/build/nsThreadUtils.cpp:238 #17 0x2b82c10be4cb in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82 #18 0x2b82c1bf4631 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:219 #19 0x2b82c1bf444e in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:186 #20 0x2b82c0e40b01 in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163 #21 0x2b82c09e916f in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:269 #22 0x2b82be99bab1 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3872 #23 0x2b82be99cddf in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3939 #24 0x2b82be99d781 in XRE_main toolkit/xre/nsAppRunner.cpp:4140 #25 0x41d1f8 in do_main(int, char**, nsIFile*) browser/app/nsBrowserApp.cpp:272 #26 0x41c72f in main browser/app/nsBrowserApp.cpp:632 #27 0x2b82bc46b76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 0x2b82d90df978 is located 6456 bytes inside of 8192-byte region [0x2b82d90de040,0x2b82d90e0040) allocated by thread T0 here: #0 0x410568 in __interceptor_malloc _asan_rtl_ #1 0x2b82bc9c9dbb in PL_ArenaAllocate nsprpub/lib/ds/plarena.c:202 I suspect this is not the same issue as in bug 787715. The use-after-poison indicates a use-after-free in our internal NSPR allocator, so marking this as s-s for now.
Comment 1•11 years ago
|
||
Hmm, didn't I fix this recently ... oh, I did: http://hg.mozilla.org/mozilla-central/rev/4ab11d8ed73b This seems to be the culprit: http://hg.mozilla.org/mozilla-central/diff/0c513ba74137/layout/xul/tree/nsTreeBodyFrame.cpp#l1.98 clearly the use of 'mHorzPosition' on line 1.117 is unsafe again now. And so is the use of 'mScrollbarActivity' on line 1.144 I can't reproduce the crash in my local ASan m-c build, can you try adding weakFrame.IsAlive() checks in those places?
Blocks: 636564
Keywords: crash,
regression
Reporter | ||
Comment 2•11 years ago
|
||
Um, can you explain me more detailed what I need to add where? I'd be glad to give it a try then.
Comment 3•11 years ago
|
||
http://hg.mozilla.org/mozilla-central/diff/0c513ba74137/layout/xul/tree/nsTreeBodyFrame.cpp#l1.98 Sure, I think we need to add back lines 1.110 - 1.112 with s/self/weakFrame/ and add "weakFrame.IsAlive() && " to the condition on line 1.144 We should probably audit all callers (recursively) to these methods to see that they are safe against arbitrary layout objects being destroyed. An alternative might be to add a nsAutoScriptBlocker in a strategic place, like we did in bug 842166.
Reporter | ||
Comment 4•11 years ago
|
||
confirmed :) With these changes, the test doesn't crash anymore.
Assignee | ||
Comment 5•11 years ago
|
||
Assignee: nobody → spohl.mozilla.bugs
Attachment #752716 -
Flags: review?(roc)
Updated•11 years ago
|
Keywords: csec-framepoisoning,
sec-other
Attachment #752716 -
Flags: review?(roc) → review+
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/59c556346255
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/59c556346255
Status: NEW → RESOLVED
Closed: 11 years ago
status-firefox24:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 8•10 years ago
|
||
How far back does this issue go? Was it trunk only?
It's in 23. We'd better uplift the fix.
Comment on attachment 752716 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 636564 User impact if declined: practically no impact since XUL isn't accessible remotely Testing completed (on m-c, etc.): just landed Risk to taking this patch (and alternatives if risky): very low risk String or IDL/UUID changes made by this patch: none
Attachment #752716 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Whiteboard: [asan][asan-test-failure] → [asan][asan-test-failure][lion-scrollbars+]
Assignee | ||
Comment 11•10 years ago
|
||
Thank you for catching this and requesting approval for aurora!
status-firefox21:
--- → unaffected
status-firefox22:
--- → unaffected
status-firefox23:
--- → affected
status-firefox-esr17:
--- → unaffected
Updated•10 years ago
|
tracking-firefox23:
--- → +
Updated•10 years ago
|
Attachment #752716 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 13•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/512e62ac59d9
Keywords: checkin-needed
Updated•10 years ago
|
Whiteboard: [asan][asan-test-failure][lion-scrollbars+] → [asan][asan-test-failure][lion-scrollbars+][adv-main23-]
Updated•10 years ago
|
status-b2g18:
--- → unaffected
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•