Closed Bug 788836 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jruderman, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: assertion, testcase)

Attachments

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

Attachment

General

Creator:
Created:
Updated:
Size: