430927 - "\x001".toSource() doesn't eval to "\x001"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[x0]

js> "\x001".toSource()
(new String("\01"))
js> eval("\x001".toSource()) == "\x001"
false
js> eval("\x001".toSource()).toSource()
(new String("\x01"))

Representation of '\0' as "\0" in strings can conflict with octal escape sequences, so it should be better "\x00", unless it can be prooven that no octal digit follows.

Comment 1

[Brian Crowder]

This is a regression; shell from the 1.8 branch does not suffer from this bug.

Comment 2

[Brian Crowder]

Regression from bug 366725, perhaps?

Comment 3

[x0]

(In reply to comment #2)
> Regression from bug 366725, perhaps?

Looking at the patch, you're probably right: js_strchr and strchr are incompatible. strchr will find the terminating '\0' while js_strchr will not. Thus, using js_EscapeMap with strchr instead of js_strchr will change behavior. And only '\0' seems to be affected, not '\x01'.

Comment 4

[Brian Crowder]

Attachment: fix

Comment 5

[Brian Crowder]

Attachment: with context this time

Comment 6

[Brian Crowder]

Attachment: a little more

Comment 7

[Igor Bukanov]

Comment on attachment 318880 [details] [diff] [review]
a little more

>-const char js_EscapeMap[] = {
>+const jschar js_EscapeMap[] = {
...
>-        if (!(c >> 8) && (e = strchr(js_EscapeMap, (int)c)) != NULL) {
>+        if (!(c >> 8) && (e = js_strchr(js_EscapeMap, c)) != NULL) {

This is not necessary. A simpler fix is to remove the last '0' from the array and use memchr(js_EscapeMap, c, sizeof(js_EscapeMap) - 1), not strchr, with comments that memchr is used to avoid matching the case of c == '\0'.

Comment 8

[Brian Crowder]

Good idea.  Coming up...

Comment 9

[Brian Crowder]

Actually, it seems like other references to js_EscapeMap may be broken by the absence of the \0 (as they might have been broken by my patch).

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Actually, it seems like other references to js_EscapeMap may be broken by the
> absence of the \0 (as they might have been broken by my patch).

I have not suggested to remove \0, I only suggested to remove the trailing '0'! This is compatible with the usage of js_EscapeMap in jsstr.c since there the code explicitly checks for '\0'.

Less minimal solution would be to change js_EscapeMap into:

extern const char       js_EscapeMap[];
#define JS_ESCAPE_MAP_LEN  9

const char js_EscapeMap[] = "\b" "\f" ... "\\"
                            "b"  "f"  ... "\\";
JS_STATIC_ASSERT(JS_ESCAPE_MAP_LEN * 2 + 1 == sizeof(js_EscapeMap));

and then use it in jsopcode.c and jsstr.c like in:

if (... (e = memchr(js_EscapeMap, c, JS_ESCAPE_MAP_LEN)) != NULL) {
... e[JS_ESCAPE_MAP_LEN].
}

Comment 11

[Brian Crowder]

Mass unowning JS bugs...  I'm not likely to be able to move these forward.

Comment 12

[Tom S [:evilpie]]

CC Brendan, i think he did some related change recently.

Comment 13

[Tom S [:evilpie]]

Attachment: v1

I think decompiling to \0 is always confusing. This patches always decompiles to \x00, and by the way fixes this bug.

Comment 14

[Tom S [:evilpie]]

Attachment: v2

Forgot to refresh.

Comment 15

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a242f11d7c58

Comment 16

[Matt Brubeck (:mbrubeck)]

Backed out because of test failures on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04505e53439e

Comment 17

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/edade842d1a8
Add back terminating \0
http://hg.mozilla.org/integration/mozilla-inbound/rev/43982bdb73dd

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/edade842d1a8
https://hg.mozilla.org/mozilla-central/rev/43982bdb73dd

please specify the bug number in follow-up patches.