[64-bit] Crash [@ RuleProcessorData::GetNthIndex] when adding <style> node to <head>

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
CSS Parsing and Computation
--
critical
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.9.1b2
x86_64
Linux
crash, fixed1.9.1, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] virtual method call on random pointer value, crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
STEPS TO REPRODUCE
1. load the attached testcase

ACTUAL RESULT
Crash occurs in local Firefox debug build on Linux (64-bit).
Crash occurs in Firefox 2008-10-22-02 nightly build (64-bit).
Crash does NOT occur in Firefox 2008-10-22-02 nightly build (32-bit).
Flags: blocking1.9.1?
(Assignee)

Comment 1

9 years ago
Created attachment 344372 [details]
Testcase
(Assignee)

Comment 2

9 years ago
Created attachment 344373 [details]
stack
(Assignee)

Comment 3

9 years ago
Regression window:  2008-08-18-05 -- 2008-08-19-05
http://hg.mozilla.org/mozilla-central/shortlog/3c518880b8d4
bug 449447 ?
Keywords: regression
(Assignee)

Updated

9 years ago
Blocks: 449447
(Assignee)

Comment 4

9 years ago
http://hg.mozilla.org/mozilla-central/file/6c4bb68efd2c/layout/style/nsCSSRuleProcessor.cpp#l1011
With a random value at *curChildPtr and then calling the virtual method
IsNodeOfType() on it this bug seems sg:critical to me.
Whiteboard: [sg:critical?] virtual method call on random pointer value
(Assignee)

Comment 5

9 years ago
Created attachment 344399 [details] [diff] [review]
Patch rev. 1

The problem is that 'childCount' is unsigned.
I found one more in SelectorMatches().
Assignee: nobody → mats.palmgren
Attachment #344399 - Flags: superreview?(bzbarsky)
Attachment #344399 - Flags: review?(bzbarsky)
So how come this wasn't a problem before?  I guess because a very large |cur| just caused us to get null and bail?

The code in ::lastNode is ok, because if index == 0 then GetChildAt() will just return null and the effect will be the same: null lastNode and ending the loop.
Comment on attachment 344399 [details] [diff] [review]
Patch rev. 1

>+++ b/layout/style/nsCSSRuleProcessor.cpp
>     stopPtr = curChildPtr - 1;
>-    curChildPtr += childCount - 1;
>+    curChildPtr += PRInt32(childCount) - 1;

I'd prefer:

  curChildPtr = stopPtr + childCount;

since that more clearly indicates that we'll walk through childCount (possibly 0) entries.
(Assignee)

Comment 8

9 years ago
(In reply to comment #6)
> I guess because a very large |cur| just caused us to get null and bail?

I would guess so too.
(Assignee)

Comment 9

9 years ago
Created attachment 344783 [details] [diff] [review]
Patch rev. 2
Attachment #344399 - Attachment is obsolete: true
Attachment #344783 - Flags: superreview?(bzbarsky)
Attachment #344783 - Flags: review?(bzbarsky)
Attachment #344399 - Flags: superreview?(bzbarsky)
Attachment #344399 - Flags: review?(bzbarsky)
Comment on attachment 344783 [details] [diff] [review]
Patch rev. 2

Please add a test.
Attachment #344783 - Flags: superreview?(bzbarsky)
Attachment #344783 - Flags: superreview+
Attachment #344783 - Flags: review?(bzbarsky)
Attachment #344783 - Flags: review+
I pushed the fix.  The testcase seems to depend on timeouts and even worse on details of the textbox binding.  Jesse, do you think you can whip together a self-contained crashtest?
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1
Reading comment 0 I assume it's 64bit only? I'm not able to reproduce it on 32bit.
Hardware: x86 → x86_64
Target Milestone: --- → mozilla1.9.1b2
Version: unspecified → Trunk
Crash Signature: [@ RuleProcessorData::GetNthIndex]
Group: core-security
You need to log in before you can comment on or make changes to this bug.