Closed Bug 461239 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

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

Crash Data

Attachments

(3 files, 1 obsolete file)

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?
Attached file Testcase
Attached file stack
Regression window:  2008-08-18-05 -- 2008-08-19-05
http://hg.mozilla.org/mozilla-central/shortlog/3c518880b8d4
bug 449447 ?
Keywords: regression
Blocks: 449447
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
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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.
(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.
Attached patch Patch rev. 2Splinter Review
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
Closed: 11 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.