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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: jruderman, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: assertion, testcase)
Attachments
(4 files, 1 obsolete file)
18 bytes,
text/html
|
Details | |
6.42 KB,
text/plain
|
Details | |
3.27 KB,
patch
|
Details | Diff | Splinter Review | |
4.63 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
###!!! ASSERTION: |First()| called on an empty string: 'mLength > 0', file nsTSubstring.h, line 240
Reporter | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 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•13 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•13 years ago
|
||
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•13 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•13 years ago
|
||
This is your suggested version with added Pushbacks per above.
Assignee: nobody → matspal
Attachment #659431 -
Attachment is obsolete: true
Assignee | ||
Comment 10•13 years ago
|
||
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•13 years ago
|
||
Thanks, I'll keep that in mind when reading it and report any issues.
Assignee | ||
Comment 13•13 years ago
|
||
Flags: in-testsuite+
Target Milestone: --- → mozilla18
Comment 14•13 years ago
|
||
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.
Description
•