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

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Created attachment 658654 [details]
testcase

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

Comment 1

[Jesse Ruderman]

Created attachment 658655 [details]
stack trace

Comment 2

[Mats Palmgren (:mats)]

I think reading the first character of an empty string is safe since
sEmptyBuffer has a zero in that slot.

Comment 3

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Not in general, since strings aren't (in general) null-terminated, so you could get anything.

Comment 4

[Mats Palmgren (:mats)]

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?

Comment 5

[Mats Palmgren (:mats)]

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.

Comment 6

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

(I think I prefer it the way I described, since it ends up with less code rather than more code.)

Comment 8

[Mats Palmgren (:mats)]

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

Comment 9

[Mats Palmgren (:mats)]

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

This is your suggested version with added Pushbacks per above.

Comment 10

[Mats Palmgren (:mats)]

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

Comment 11

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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

Comment 12

[Mats Palmgren (:mats)]

Thanks, I'll keep that in mind when reading it and report any issues.

Comment 13

[Mats Palmgren (:mats)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1a98daab06

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1a1a98daab06