Closed Bug 430927 Opened 12 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: x00000000, Assigned: evilpie)

References

()

Details

Attachments

(2 files, 3 obsolete files)

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.
This is a regression; shell from the 1.8 branch does not suffer from this bug.
Regression from bug 366725, perhaps?
(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'.
Attached patch fix (obsolete) — Splinter Review
Assignee: general → crowder
Status: NEW → ASSIGNED
Attachment #318876 - Flags: review?(igor)
Attached patch with context this time (obsolete) — Splinter Review
Attachment #318876 - Attachment is obsolete: true
Attachment #318877 - Flags: review?(igor)
Attachment #318876 - Flags: review?(igor)
Attached patch a little moreSplinter Review
Attachment #318877 - Attachment is obsolete: true
Attachment #318880 - Flags: review?(igor)
Attachment #318877 - Flags: review?(igor)
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'.
Good idea.  Coming up...
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).
(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].
}

Attachment #318880 - Flags: review?(igor)
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
Attached patch v1 (obsolete) — Splinter Review
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)
Attached patch v2Splinter Review
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+
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
Closed: 8 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.