Closed Bug 1138092 Opened 5 years ago Closed 5 years ago

Crash at xul!IsRubyAlignSpaceAround

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- unaffected
firefox37 --- unaffected
firefox38 + fixed
firefox39 --- fixed
firefox-esr31 --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- fixed

People

(Reporter: fuzzers.con.amor, Assigned: xidorn, NeedInfo)

References

Details

(Keywords: regression)

Crash Data

Attachments

(3 files)

Attached file Proof of concept
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36

Steps to reproduce:

1. Upload the attached sample to a local webserver.
2. Open Firefox Nightly and navigate to the sample file.


Actual results:

Crash at xul!IsRubyAlignSpaceAround


Expected results:

No crash.

Notes:

- It seems a null ptr dereference but marked as security until it's confirmed by an expert.

- Tested on:
 
 * Firefox Nightly Builds 39.0a1 (2015-02-28). It does not repro in Firefox 36.
 * WindowsXP, Win7 and Win8.1.
Attached file Stack trace
stacktrace attached
I do see one report of the crash @IsRubyAlignSpaceAround in crash stats from a Feb 28 build
bp-ac47ec95-d18b-497e-aaf5-1578c2150228

Today I crash with a similar stack in the function above, @nsLineLayout::TextAlignLine 
bp-174ba9d2-8271-4b2d-8cf8-b28122150302

Both appear to be null derefs, but I can't tell how seriously we're confused to get to that point and whether a more useful (to an attacker) crash could be produced.

The style tags don't appear to be necessary to the testcase. This works:
  data:text/html,<html><rt><canvas><fieldset></fieldset><rb></html>


Jesse: are ruby-related tags being picked up in your fuzzer? If not is there a problem with the fuzzer or is it a lack of source material in the testsuites? It seems to be a big chunk of new layout code which is always scary.
Status: UNCONFIRMED → NEW
Crash Signature: [@ IsRubyAlignSpaceAround ] [@ nsLineLayout::TextAlignLine(nsLineBox*, bool) ]
Component: Untriaged → Layout
Ever confirmed: true
Flags: needinfo?(jruderman)
Product: Firefox → Core
Keywords: regression
This crash is a regression of bug 1055676, but the intrinsic problem is something broken from the beginning.
OS: Windows 8.1 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
I have no idea how to fix this problem.

The problem here is that, some HTML elements may generate an anonymous box to wrap its children, even if it has an inline display, especially form controls. This breaks the concept of IsInlineDescendantOfRuby as well as the style computation depending on it. It means we need to know the frame construction data before we can compute it.

heycam suggested me that I can add a flag during constructing frames to notify the style context. But the problem is that style context could be created very early. During dynamic changes, children of a context style may even be created earlier than the frame construction item.

For this very bug, though, the crash can be fixed by adding some sanity check to the conditions. But the true problem doesn't seem to have an easy solution.

dbaron, any idea?
Flags: needinfo?(dbaron)
I honestly wouldn't worry too much about being correct here; the ruby spec is doing pretty crazy things, architecturally, that other layout specs simply haven't done, and I don't think we should worry that much about edge cases like form controls inside of ruby.  It just shouldn't crash.
Flags: needinfo?(dbaron)
Attached patch patchSplinter Review
Assignee: nobody → quanxunzhen
Attachment #8572219 - Flags: review?(dbaron)
Attachment #8572219 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/f2d3a70404cf
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8572219 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1055676
[User impact if declined]: crash for some content
[Describe test coverage new/current, TreeHerder]: will add crashtest
[Risks and why]: no risk, only add a sanity check to avoid crash
[String/UUID change made/needed]: n/a
Attachment #8572219 - Flags: approval-mozilla-aurora?
[Tracking Requested - why for this release]: 

See comment 9
Attachment #8572219 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.