752230 - control characters above 0x7e should not be allowed in unquoted url()

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: test case

CSS2.1 has

URI 	url\({w}{string}{w}\)
        |url\({w}([!#$%&*-\[\]-~]|{nonascii}|{escape})*{w}\)

nonascii	[^\0-\237]

where \237 is the octal representation of 0x9f and the last control character of all the u+??

This is a leftout case of bug 604179 patch 5. I have a patch already.

Comment 1

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: proposed fix

I was originally trying to improve performance of url() tokenization by expanding the lexing table, but I don't think I am experienced enough to say this is really gaining performance. Fixing this is just an accident. :p

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 621308 [details] [diff] [review]
proposed fix

>+#define SI   IS_IDENT|START_IDENT
>+#define XI   IS_IDENT            |IS_HEX_DIGIT

>+#define XSI  IS_IDENT|START_IDENT|IS_HEX_DIGIT

These three don't look like they're needed anymore (though maybe I missed something), so remove them.


> static const PRUint8 gLexTable[] = {
>-//                                     TAB LF      FF  CR
>-   0,  0,  0,  0,  0,  0,  0,  0,  0,  W,  W,  0,  W,  W,  0,  0,
>+//                                              TAB  LF        FF   CR
>+   0,   0,   0,   0,   0,   0,   0,   0,   0,   W,   W,   0,   W,   W,   0,   0,

I'd rather you not expand the table width, since it pushes it over 80 characters.  Just make the line with ABCDEF break the alignment instead.

>+static inline bool
>+IsURLChar(PRInt32 ch) {
>+  return ch >= 256 || (gLexTable[ch] & IS_URL_CHAR) != 0;
>+}

This needs to check ch >= 0, just like IsIdent does.  In fact, the function should look exactly like IsIdent except for checking a different bit.


r=dbaron with those changes

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 621308 [details] [diff] [review]
proposed fix

Review of attachment 621308 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSScanner.cpp
@@ +947,4 @@
>      }
> +
> +    ch = Read();
> +    if (ch < 0) break;

if (ch < 0) {
  break;
}

Comment 4

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: comments addressed

but I identified a flaw so I am asking for a check.

Comment 5

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to David Baron [:dbaron] from comment #2)
> Comment on attachment 621308 [details] [diff] [review]
> proposed fix
> 
> >+#define SI   IS_IDENT|START_IDENT
> >+#define XI   IS_IDENT            |IS_HEX_DIGIT
> 
> >+#define XSI  IS_IDENT|START_IDENT|IS_HEX_DIGIT
> 
> These three don't look like they're needed anymore (though maybe I missed
> something), so remove them.

Ah, yeah, I missed that. And I realized I didn't change SI to USI for U+A0-U+FF. This is fixed in the updated patch (with a test). I am asking for a check because of this.

Also, I forgot the #undef's.
 
> > static const PRUint8 gLexTable[] = {
> >-//                                     TAB LF      FF  CR
> >-   0,  0,  0,  0,  0,  0,  0,  0,  0,  W,  W,  0,  W,  W,  0,  0,
> >+//                                              TAB  LF        FF   CR
> >+   0,   0,   0,   0,   0,   0,   0,   0,   0,   W,   W,   0,   W,   W,   0,   0,
> 
> I'd rather you not expand the table width, since it pushes it over 80
> characters.  Just make the line with ABCDEF break the alignment instead.

Done.

> >+static inline bool
> >+IsURLChar(PRInt32 ch) {
> >+  return ch >= 256 || (gLexTable[ch] & IS_URL_CHAR) != 0;
> >+}
> 
> This needs to check ch >= 0, just like IsIdent does.  In fact, the function
> should look exactly like IsIdent except for checking a different bit.

I actually examined this pretty clearly and IsURLChar is the only function where the ch >= 0 check isn't necessary. But anyway I am adding this check back for now and hopefully someone in the future can refactor the whole file somehow.


(In reply to Ms2ger from comment #3)
> Comment on attachment 621308 [details] [diff] [review]
> proposed fix
> 
> Review of attachment 621308 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsCSSScanner.cpp
> @@ +947,4 @@
> >      }
> > +
> > +    ch = Read();
> > +    if (ch < 0) break;
> 
> if (ch < 0) {
>   break;
> }

Done (although various places in this file still have the former style). I should spend some time reading the style guideline.

Comment 6

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: interdiff

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 622259 [details] [diff] [review]
comments addressed

r=dbaron

(Ah, the mistake was missing the U bit for U+00A0 to U+00FF.)

> // @   A   B   C   D   E   F   G   H   I   J   K   L   M   N   O
>-   0,  XSI,XSI,XSI,XSI,XSI,XSI,SI, SI, SI, SI, SI, SI, SI, SI, SI,
>+U,UXSI,UXSI,UXSI,UXSI,UXSI,UXSI,USI,USI,USI,USI,USI,USI,USI,USI,USI,

Do re-align the comment to match the values for this line, though.

Sorry for the delay.

Comment 8

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: final patch

Carry over r=dbaron

Comment 9

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to David Baron [:dbaron] from comment #7)
> > // @   A   B   C   D   E   F   G   H   I   J   K   L   M   N   O
> >-   0,  XSI,XSI,XSI,XSI,XSI,XSI,SI, SI, SI, SI, SI, SI, SI, SI, SI,
> >+U,UXSI,UXSI,UXSI,UXSI,UXSI,UXSI,USI,USI,USI,USI,USI,USI,USI,USI,USI,
> 
> Do re-align the comment to match the values for this line, though.

Ah, right. Done. (I indented those two lines again and they have 71 characters. I think this is in the acceptable range.)

> Sorry for the delay.

That was no delay as compared to my experience sending patch to WebKit! I am pretty sure most people on #whatwg share similar experience interacting with Mozilla folks on Bugzilla.

Comment 10

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14abca4e7378

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/14abca4e7378

Comment 12

[Frédéric Bourgeon [:FredB]]

The documentation has been updated with a note on the syntax as can be read here: https://developer.mozilla.org/en-US/docs/CSS/uri#Syntax

If that's enough, then this bug can be marked as dev-doc-complete.

Comment 13

[Florian Scholz (Open Web Docs)]

(In reply to Frédéric Bourgeon [:FredB] from comment #12)
> The documentation has been updated with a note on the syntax as can be read
> here: https://developer.mozilla.org/en-US/docs/CSS/uri#Syntax
> 
> If that's enough, then this bug can be marked as dev-doc-complete.

I don't think this will need more documentation. Marking as complete.