Closed Bug 788836 Opened 12 years ago Closed 12 years ago

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


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: jruderman, Assigned: MatsPalmgren_bugz)



(Keywords: assertion, testcase)


(4 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: |First()| called on an empty string: 'mLength > 0', file nsTSubstring.h, line 240
Attached file stack trace
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.
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?
Attached patch fix+crashtest (obsolete) — Splinter Review
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]

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.)
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:


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


which today (correctly) tokenize as:

will tokenize as:

which would be wrong per
Group: core-security
Severity: normal → minor
OS: Mac OS X → All
Hardware: x86_64 → All
This is your suggested version with added Pushbacks per above.
Assignee: nobody → matspal
Attachment #659431 - Attachment is obsolete: true
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.
Attachment #659867 - Flags: review?(dbaron)
Comment on attachment 659867 [details] [diff] [review]
fix+crashtest, v3


(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+
Thanks, I'll keep that in mind when reading it and report any issues.
Flags: in-testsuite+
Target Milestone: --- → mozilla18
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.