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

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
--
minor
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: x0, Assigned: evilpie)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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

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

Comment 2

9 years ago
Regression from bug 366725, perhaps?
(Reporter)

Comment 3

9 years ago
(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

9 years ago
Created attachment 318876 [details] [diff] [review]
fix
Assignee: general → crowder
Status: NEW → ASSIGNED
Attachment #318876 - Flags: review?(igor)

Comment 5

9 years ago
Created attachment 318877 [details] [diff] [review]
with context this time
Attachment #318876 - Attachment is obsolete: true
Attachment #318877 - Flags: review?(igor)
Attachment #318876 - Flags: review?(igor)

Comment 6

9 years ago
Created attachment 318880 [details] [diff] [review]
a little more
Attachment #318877 - Attachment is obsolete: true
Attachment #318880 - Flags: review?(igor)
Attachment #318877 - Flags: review?(igor)

Comment 7

9 years ago
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

9 years ago
Good idea.  Coming up...

Comment 9

9 years ago
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

9 years ago
(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].
}

Updated

9 years ago
Attachment #318880 - Flags: review?(igor)

Comment 11

8 years ago
Mass unowning JS bugs...  I'm not likely to be able to move these forward.
Assignee: crowder → general
Status: ASSIGNED → NEW
CC Brendan, i think he did some related change recently.
Assignee: general → evilpies
Created attachment 570544 [details] [diff] [review]
v1

I think decompiling to \0 is always confusing. This patches always decompiles to \x00, and by the way fixes this bug.
Attachment #570544 - Flags: review?(jwalden+bmo)
Created attachment 570548 [details] [diff] [review]
v2

Forgot to refresh.
Attachment #570544 - Attachment is obsolete: true
Attachment #570548 - Flags: review?(jwalden+bmo)
Attachment #570544 - Flags: review?(jwalden+bmo)
Attachment #570548 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/a242f11d7c58
Backed out because of test failures on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04505e53439e
http://hg.mozilla.org/integration/mozilla-inbound/rev/edade842d1a8
Add back terminating \0
http://hg.mozilla.org/integration/mozilla-inbound/rev/43982bdb73dd
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Duplicate of this bug: 537849
You need to log in before you can comment on or make changes to this bug.