"ASSERTION: |First()| called on an empty string" (nsCSSParser.cpp - NonMozillaVendorIdentifier)

RESOLVED FIXED in mozilla18

Status

()

Core
CSS Parsing and Computation
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla18
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 658654 [details]
testcase

###!!! ASSERTION: |First()| called on an empty string: 'mLength > 0', file nsTSubstring.h, line 240
(Reporter)

Comment 1

5 years ago
Created attachment 658655 [details]
stack trace
(Assignee)

Comment 2

5 years ago
I think reading the first character of an empty string is safe since
sEmptyBuffer has a zero in that slot.
Not in general, since strings aren't (in general) null-terminated, so you could get anything.
(Assignee)

Comment 4

5 years ago
OK, I was thinking of the strings that the CSS parser can create.
AFAIK, it only uses string types where SetLength(0) results in
mData being == sEmptyBuffer.  Am I mistaken?
(Assignee)

Comment 5

5 years ago
Created attachment 659431 [details] [diff] [review]
fix+crashtest

The problem appears to be in nsCSSScanner::GatherIdent.
ParseAtKeyword pass aChar == 0 so we handle the \
inside the loop, which returns true also when it fails.
I think it should return false when aIdent.Length() == 0,
the same as when handling aChar == \ at the top.

In other words, GatherIdent should return false when the
ident is empty.

For a non-zero length, e.g. <style>@blah\</style>
it seems correct to return true, with the \ pushback'ed.
That is, we got a valid "blah" at-keyword token, with some
junk after it.
Attachment #659431 - Flags: review?(dbaron)
Comment on attachment 659431 [details] [diff] [review]
fix+crashtest

Seems reasonable, though I think it would be simpler to:

 * pass nextChar to ParseAtKeyword instead of removing the param (and remove the Pushback(nextChar), and replace the followingChar variable with just a Peek() call inlined)
 * make ParseAtKeyword pass it along to GatherIdent
 * make GatherIdent assert that the char it's given is not 0 (since this is the only caller that ever passes 0)
 * not change the break in GatherIdent to a return

r=dbaron either way, though


I think we can probably open this bug; it is a guaranteed read of the null terminator in this case.
Attachment #659431 - Flags: review?(dbaron) → review+
(I think I prefer it the way I described, since it ends up with less code rather than more code.)
(Assignee)

Comment 8

5 years ago
Your suggested alternative fix results in a functional change to
the tokenization though, which I assume is not intended.

We must Pushback(nextChar) in the case StartsIdent fails and
ParseAtKeyword is not called.  Otherwise this test fails:

<style>*{color:red}@{}*{color:green}</style>
<p>GREEN</p>

Secondly, with your suggested changes, we must also Pushback(aChar)
inside ParseAtKeyword in the case GatherIdent fails (on a leading
escape), otherwise this test:

<style>@\</style>

which today (correctly) tokenize as:
symbol('@')
symbol('\\')
EOF

will tokenize as:
symbol('@')
EOF

which would be wrong per
http://dev.w3.org/csswg/css3-syntax/#at-keyword-state
http://dev.w3.org/csswg/css3-syntax/#data-state0
Group: core-security
Severity: normal → minor
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Comment 9

5 years ago
Created attachment 659858 [details] [diff] [review]
fix+crashtest, v2

This is your suggested version with added Pushbacks per above.
Assignee: nobody → matspal
Attachment #659431 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 659867 [details] [diff] [review]
fix+crashtest, v3

I prefer this version since it's easier to understand when ParseAtKeyword
does all the parsing/error handling in one place.  It corresponds the
spec At-keyword/-rest states.

https://tbpl.mozilla.org/?tree=Try&rev=322c36810d71
Attachment #659867 - Flags: review?(dbaron)
Comment on attachment 659867 [details] [diff] [review]
fix+crashtest, v3

r=dbaron


(But beware, css3-syntax isn't really ready for use yet.  Parts of it were reverse-engineered from WebKit in cases where WebKit was the only browser *not* matching CSS 2.1.)
Attachment #659867 - Flags: review?(dbaron) → review+
(Assignee)

Comment 12

5 years ago
Thanks, I'll keep that in mind when reading it and report any issues.
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1a98daab06
Flags: in-testsuite+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/1a1a98daab06
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.