Closed Bug 1001994 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect, P3, critical)

defect

Tracking

()

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

People

(Reporter: martijn.martijn, Assigned: mats)

References

Details

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

Crash Data

Attachments

(6 files)

Attached file crash1.htm
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
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
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
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 fixSplinter 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.
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-highsec-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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Whiteboard: [adv-main33+]
Confirmed crash in Fx31 release.
Verified fixed in Fx33, release candidate.
Blocks: 5588
Group: core-security → core-security-release
Group: core-security-release
Depends on: 1403117
No longer depends on: 1403117
Duplicate of this bug: 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.