nth-child integer overflow with crash in [@ nthChildGenericMatches ]

RESOLVED FIXED in Firefox 44

Status

()

--
critical
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: abbGZcvu_bugzilla.mozilla.org, Assigned: dbaron)

Tracking

({crash, reproducible, testcase})

Trunk
mozilla44
crash, reproducible, testcase
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

(crash signature, URL)

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150826023504

Steps to reproduce:

Load the following html in Firefox
<style>*:nth-child(-n-2147483647){



Actual results:

Integer overflow exception


Expected results:

Nothing.

Comment 1

3 years ago
Created attachment 8663289 [details]
1206105.html (instant crash)

Comment 2

3 years ago
CR: https://crash-stats.mozilla.com/report/index/a48e8639-f0d4-479e-b19a-c0f3f2150919
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nthChildGenericMatches ]
Component: Untriaged → Layout
Ever confirmed: true
Keywords: crash, reproducible, testcase
Product: Firefox → Core
Summary: nth-child integer overflow → nth-child integer overflow with crash in [@ nthChildGenericMatches ]
Is it new that this is a crash rather than regular old "undefined behavior".
Component: Layout → CSS Parsing and Computation
Version: 40 Branch → Trunk

Comment 4

3 years ago
FF8 crashes so it is more an old undefined case.
Odd. I get a crash on Mac too:

  Program received signal EXC_ARITHMETIC, Arithmetic exception.

The debugger claims we're on this line:

  const int32_t n = (index - b) / a;

with index == 1, b == -2147483647 (so -INT32_MAX) and a == -1.  All three variables are int32_t.

The subtraction index - b is in fact undefined behavior here, since it's basically 1+INT32_MAX.  But what compilers seem to do with it in practice is just let it overflow in the obvious way, so we get INT32_MIN.

It's the division of INT32_MIN by -1 that raises EXC_ARITHMETIC.  That's probably undefined behavior too, though I haven't checked....
Anyway, seems like we should just use a CheckedInt32 here, and "not match" if it overflows?  And check index-b for being INT32_MIN before dividing by -1.
Flags: needinfo?(dbaron)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Created attachment 8664087 [details] [diff] [review]
Use CheckedInt for an+b selector matching
Attachment #8664087 - Flags: review?(bzbarsky)
Comment on attachment 8664087 [details] [diff] [review]
Use CheckedInt for an+b selector matching

r=me
Attachment #8664087 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/000fe1df3e59
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.