Closed Bug 1001994 Opened 11 years ago Closed 10 years ago

crash in libsystem_kernel.dylib@0x15866 on printing with position: sticky

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla33
Tracking Flags:
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- verified
firefox-esr24 --- unaffected
firefox-esr31 --- wontfix
b2g18 --- unaffected

People

(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)

References

Details

(4 keywords, Whiteboard: [adv-main33+])

Crash Data

Attachments

(6 files)

crash1.htm
(deleted), text/html
Details
crash stack on Linux
(deleted), text/plain
Details
crash_reduced.htm
(deleted), text/html
Details
framedump + stack
(deleted), text/html
Details
fix
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
crashtest patch - land after the bug is public
(deleted), patch
Details | Diff | Splinter Review
Attached file crash1.htm (deleted) —
See testcase, which crashes on printing or print preview. This bug was filed from the Socorro interface and is report bp-44611da7-eede-4bd8-98e8-73db42140427. ============================================================= 0 libsystem_kernel.dylib libsystem_kernel.dylib@0x15866 1 libsystem_pthread.dylib libsystem_pthread.dylib@0x235c 2 libc++abi.dylib libc++abi.dylib@0x27726 3 libsystem_c.dylib libsystem_c.dylib@0x5cb1a 4 libc++abi.dylib libc++abi.dylib@0x27726 5 libc++abi.dylib libc++abi.dylib@0xf31 6 XUL nsStyleContext::~nsStyleContext() obj-firefox/x86_64/dist/include/nsCOMPtr.h 7 libc++abi.dylib libc++abi.dylib@0x25447 8 XUL nsLayoutUtils::GetNearestScrollableFrame(nsIFrame*, unsigned int) layout/generic/nsQueryFrame.h 9 XUL mozilla::StickyScrollContainer::GetStickyScrollContainerForFrame(nsIFrame*) layout/generic/StickyScrollContainer.cpp 10 XUL nsFrame::DestroyFrom(nsIFrame*) layout/generic/nsFrame.cpp
Priority: -- → P3
Summary: crash in libsystem_kernel.dylib@0x15866 on printing → crash in libsystem_kernel.dylib@0x15866 on printing with position: sticky
This, like bug 973971, happens while processing a C++ exception.
We actually don't support C++ exceptions, though they can still happen anyway. See bug 975158. We'd presumably crash on *any* C++ exception. And probably the only way to "fix" a bug like this is to avoid throwing the exception to begin with (to find out why it's being thrown).
Unfortunately I can't reproduce this crash. I tested with today's m-c nightly on OS X 10.7.5, 10.8.5 and 10.9.2.
atos translation of top part of stack from comment #0: __pthread_kill (in libsystem_kernel.dylib) + 10 pthread_kill (in libsystem_pthread.dylib) + 92 char const* __cxxabiv1::(anonymous namespace)::parse_block_invoke<__cxxabiv1:: (anonymous namespace)::Db>(char const*, char const*, __cxxabiv1::(anonymous namespace)::Db&) ::test (in libc++abi.dylib) + 1129 abort (in libsystem_c.dylib) + 125 char const* __cxxabiv1::(anonymous namespace)::parse_block_invoke<__cxxabiv1:: (anonymous namespace)::Db>(char const*, char const*, __cxxabiv1::(anonymous namespace)::Db&) ::test (in libc++abi.dylib) + 1129 __cxa_bad_cast (in libc++abi.dylib) + 0 nsStyleContext::~nsStyleContext() __cxa_deleted_virtual (in libc++abi.dylib) + 0 ...
Group: core-security
Attached file crash stack on Linux (deleted) —
It looks like we're calling a virtual method on a deallocated frame. STR: Print Preview the attached testcase, change the Scale between various % values a few times.
Corey, maybe you can take a look?
Flags: needinfo?(corey)
+cc: kip
I hadn't noticed that this was reproducible on Linux, but I can reproduce it per comment 5. What I see happening in the stack traces is: when destroying the sticky frame, we try to unregister it from its StickyScrollContainer. That requires first finding the scroll container frame, which I think in this document ought to be the nsHTMLScrollFrame corresponding to <html>, but I've no idea how that works in printing and gdb is being slightly uncooperative. Anyway, while walking up the frame tree in search of the scroll container, we hit that deallocated frame. I guess I would expect frame trees to be destroyed starting from the leaves, but maybe that doesn't hold here?
Flags: needinfo?(corey)
Attached file crash_reduced.htm (deleted) —
After removing some things from the file, I'm still able to reliably reproduce the crash by keyboard-scrolling through all the percentage scale options while print-previewing.
"Print Preview" is not a super-effective attack vector (sec-moderate?) but if we're screwing up the frame tree could it be made to happen in a non-print context? Often that seems to be the case, in which case this could be sec-critical. Jesse: do we hit position:sticky in you fuzzer often?
Flags: needinfo?(jruderman)
Yes, my fuzzer uses position:sticky all the time. But I've disabled testing of printing because my printing bugs never get fixed.
Flags: needinfo?(jruderman)
Can someone please take this issue and work on a patch?
Assignee: nobody → mats
Attached file framedump + stack (deleted) —
The position:sticky frame is on an OverflowList of an inline, which may in some cases have a bogus parent frame pointer (because we reparent those lazily during reflow for performance reasons). But nsIFrame::DestroyFrom now depends on having a valid parent frame pointer chain to find the sticky controller: http://hg.mozilla.org/mozilla-central/annotate/bcab7bc926c5/layout/generic/nsFrame.cpp#l594
Attached patch fix (deleted) — — Splinter Review
The "if (overflowFrames)" check isn't needed but I think it's an optimization because it should rarely be non-null here. The eForDestroy value isn't strictly needed, since I could just pass zero and avoid the ReparentStyleContext part, but it's for documenting this case if more code is added here later that we could avoid for eForDestroy. This fixes the crash for me locally - I'll push to Try later after approval-to-land.
Attachment #8450605 - Flags: review?(roc)
Comment on attachment 8450553 [details] framedump + stack After debugging this a bit more I can see that the parent pointer we crash on is "Inline(span)(1)@7fffb9607550" (the first child of the "Block(ul)(1)@7fffb9606aa8"). So since that frame should have been destroyed normally, I'm a bit puzzled why it isn't poisoned...
OK, we're deleting the whole shell so we're not poisoning anything since it will free the whole arena soon anyway. So, the frame should be intact except for what the destructor does.
In both DEBUG and non-DEBUG builds on Linux64 I get the "pure virtual method called" which calls std::terminate() which raises an exception, which is fatal. Comment 1 to 4 sounds like the same happens on OSX. So those should be non-exploitable. I haven't tried Windows.
OS: Mac OS X → All
mozilla26 and later are affected. I don't know how to set "status-b2g-v*" because it's unclear to me what branches those are based on, ditto for status-seamonkey2.26.
Blocks: 886646
status-b2g18: --- → unaffected
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox-esr24: --- → unaffected
Keywords: regression
In a trunk DEBUG build on Windows I get "pure virtual function call" followed by a crash. FF30 on Windows does not crash at all.
I think it's very unlikely that this would be exploitable.
Keywords: sec-high → sec-low
Comment on attachment 8450605 [details] [diff] [review] fix [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily. Besides, it seems very unlikely to be exploitable anyway. I'm just asking for sec-approval in case I missed something above. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? The bug affects mozilla26 and later. If not all supported branches, which bug introduced the flaw? bug 886646 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply, if needed. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause problems.
Attachment #8450605 - Flags: sec-approval?
Comment on attachment 8450605 [details] [diff] [review] fix It is a sec-low so it doesn't need sec-approval to land. Only High and Critical security bugs do. Check it in!
Attachment #8450605 - Flags: sec-approval?
Isn't it odd that we've completed reflow and there's still something on the overflow list?
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #25) > Isn't it odd that we've completed reflow and there's still something on the > overflow list? Indeed it is; I'm aware of it and made a patch along those lines but I figured it wasn't the root cause of this bug and would only wallpaper it. Filed bug 1035299.
https://hg.mozilla.org/mozilla-central/rev/a5d6027756ae
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox33: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Whiteboard: [adv-main33+]
Confirmed crash in Fx31 release. Verified fixed in Fx33, release candidate.
Status: RESOLVED → VERIFIED
status-firefox31: affected → wontfix
status-firefox32: affected → wontfix
status-firefox33: fixed → verified
Blocks: 5588
Group: core-security → core-security-release
Group: core-security-release
Depends on: 1403117
No longer depends on: 1403117
Depends on: 1403117
Depends on: 1382213
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: