247179 - \b and \w assertions recognises non-ASCII alphanumerics as word characters

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Malcolm Rowe]

per ECMA-262 15.10.2.6, the \b assertion should break at 'word' boundaries,
where 'word' means characters A-Za-z0-9_ and no others. It seems that the
current implementation includes other characters (see example URL).

js/src/jsregexp.c includes the following comments (line 930ff or so):
 *              '\b'                    Word boundary (between \w and \W).
 *              '\w'                    A word character, [0-9a-z_A-Z].

This is happening because JS_ISWORD is based on JS_ISALNUM, which is true for
non-ASCII alphabetics. Nothing else seems to use JS_ISALNUM.

I think that JS_ISWORD should probably not be using JS_CTYPE to determine
whether a character is a 'word' character. Rather, it should probably check the
allowed characters specifically. JS_ISALNUM would then no longer be necessary.

Comment 1

[Malcolm Rowe]

Attachment: Change JS_ISWORD to disallow non-ASCII alphabetics

Something like this? (disclaimer: compiles, not tested).

I forgot to mention: IE currently appears to follow the ECMA-262 spec, as far
as this behaviour is concerned. Should we change to match IE and ECMA-262?

Comment 2

[Brendan Eich [:brendan]]

Seems real enough -- wonder whether the ECMA spec reflects the intentions of the
technical group, though.  Cc'ing waldemar.

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 150975 [details] [diff] [review]
Change JS_ISWORD to disallow non-ASCII alphabetics

No need to move the #define down.

Also, do you want JS_ISDIGIT or JS7_ISDEC?

/be

Comment 4

[Malcolm Rowe]

> No need to move the #define down.
Ok, just thought it would read better if it depended only upon macros that were 
fully defined at point-of-definition. I know it doesn't make a difference.

> Also, do you want JS_ISDIGIT or JS7_ISDEC?
Ah, didn't see JS7_ISDEC - yes, that would be right.

I'll generate a new patch tonight, if required. Hopefully, I'll also be able to 
test it this time, too (my tree's broken at the moment).

I probably should have mentioned that the URL is encoded as UTF-8, though as it 
happens, I don't think it's making any difference.

Comment 5

[Malcolm Rowe]

Attachment: Updated patch

Updated according to brendan's comments. I still can't test this; my tree is
shot.

Comment 6

[Malcolm Rowe]

Comment on attachment 151052 [details] [diff] [review]
Updated patch

My tree is happier now: the patch seems to work as intended.

I also ran the JS regression suite with no differences between pre-patch and
post-patch.

Comment 7

[Brendan Eich [:brendan]]

Attachment: patch based on Malcom's patch

This cleans up a nearby comment style nit, and avoids repeating the (c) < 128
test (although any decent compiler should common it).

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 151052 [details] [diff] [review]
Updated patch

Looks good, but I'm gonna check in the next patch, crediting Malcolm and
marking r=brendan.  Hope that's ok.

/be

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 151173 [details] [diff] [review]
patch based on Malcom's patch

Checking this in now.

/be

Comment 10

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-247179.js

Malcolm, with your permission this will be included in the javascript test
library.

Comment 12

[Malcolm Rowe]

Comment on attachment 174931 [details]
js1_5/Regress/regress-247179.js

Fine by me, although for proper attribution I should probably note that the
original example came from Jim Ley, in the context of the WHATWG discussions.

Comment 13

[Bob Clary [:bc] (inactive)]

Malcolm, can you send me Jim's email?

Comment 14

[Malcolm Rowe]

(In reply to comment #13)
> Malcolm, can you send me Jim's email?

Done.

Comment 15

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-247179.js checked in.

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.x and trunk.