Last Comment Bug 430927 - "\x001".toSource() doesn't eval to "\x001"
: "\x001".toSource() doesn't eval to "\x001"
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla10
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
javascript:'\x001'.toSource()
: 537849 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-26 02:11 PDT by x0
Modified: 2011-11-29 15:49 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (344 bytes, patch)
2008-05-01 13:25 PDT, Brian Crowder
no flags Details | Diff | Review
with context this time (957 bytes, patch)
2008-05-01 13:26 PDT, Brian Crowder
no flags Details | Diff | Review
a little more (3.56 KB, patch)
2008-05-01 13:32 PDT, Brian Crowder
no flags Details | Diff | Review
v1 (2.33 KB, patch)
2011-10-30 07:06 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Review
v2 (2.51 KB, patch)
2011-10-30 07:29 PDT, Tom Schuster [:evilpie]
jwalden+bmo: review+
Details | Diff | Review

Description x0 2008-04-26 02:11:04 PDT
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 2008-04-28 10:00:46 PDT
This is a regression; shell from the 1.8 branch does not suffer from this bug.
Comment 2 Brian Crowder 2008-04-28 10:13:08 PDT
Regression from bug 366725, perhaps?
Comment 3 x0 2008-04-28 12:28:17 PDT
(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 2008-05-01 13:25:47 PDT
Created attachment 318876 [details] [diff] [review]
fix
Comment 5 Brian Crowder 2008-05-01 13:26:28 PDT
Created attachment 318877 [details] [diff] [review]
with context this time
Comment 6 Brian Crowder 2008-05-01 13:32:23 PDT
Created attachment 318880 [details] [diff] [review]
a little more
Comment 7 Igor Bukanov 2008-05-01 13:51:40 PDT
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 2008-05-01 13:53:55 PDT
Good idea.  Coming up...
Comment 9 Brian Crowder 2008-05-01 14:02:07 PDT
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 2008-05-01 14:21:00 PDT
(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 2009-05-04 10:29:49 PDT
Mass unowning JS bugs...  I'm not likely to be able to move these forward.
Comment 12 Tom Schuster [:evilpie] 2011-01-05 09:40:10 PST
CC Brendan, i think he did some related change recently.
Comment 13 Tom Schuster [:evilpie] 2011-10-30 07:06:52 PDT
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.
Comment 14 Tom Schuster [:evilpie] 2011-10-30 07:29:37 PDT
Created attachment 570548 [details] [diff] [review]
v2

Forgot to refresh.
Comment 16 Matt Brubeck (:mbrubeck) 2011-11-03 15:26:43 PDT
Backed out because of test failures on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04505e53439e
Comment 18 Marco Bonardo [::mak] 2011-11-05 02:51:59 PDT
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.
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-29 15:49:24 PST
*** Bug 537849 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.