Last Comment Bug 366601 - switch assumes all atom indexes below 64K
: switch assumes all atom indexes below 64K
Status: VERIFIED FIXED
[sg:nse] hints at similar bugs 365692...
: verified1.8.0.10, verified1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-10 12:49 PST by Igor Bukanov
Modified: 2007-08-08 07:54 PDT (History)
1 user (show)
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Minimal fix v1 (1.27 KB, patch)
2007-01-10 12:52 PST, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
Minimal fix v2 (1.27 KB, patch)
2007-01-11 02:40 PST, Igor Bukanov
igor: review+
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review
Minimal fix v2b (1.28 KB, patch)
2007-01-16 15:05 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
js1_5/Regress/regress-366601.js (2.59 KB, text/plain)
2007-01-17 04:06 PST, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2007-01-10 12:49:57 PST
The bytecode generator for the lookup form of the switch assumes that atom indexes  always fits 2 bytes. The following example that builds a code with over 64K strings demonstrates the problem with jsshell:

~/m/trunk/mozilla/js/src> cat ~/m/files/y4.js
var N = 100*1000;
var src = 'var x = ["';
var array = Array(N);
for (var i = 0; i != N; ++i)
  array[i] = i;
src += array.join('","')+'"];\n';
src += 'switch (a) { case "a": case "b": case "c": return null; }  return x;';
var f = Function('a', src);
var r = f("a");
if (r !== null)
  throw "Unexpected result: bad switch label";
~/m/trunk/mozilla/js/src> 
~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y4.js 
uncaught exception: Unexpected result: bad switch label
 
This is not exploitable as the interpreter checks for the atom type explicitly and can deal with any atom type. Similarly the decompiler uses a generic code to print atoms so this is safe. Still I keep the bug private as it strongly hints at similar problems that are exploitable, see bug 365692, bug 366122.
Comment 1 Igor Bukanov 2007-01-10 12:52:29 PST
Created attachment 251100 [details] [diff] [review]
Minimal fix v1

A minimal fix to use a conditional switch for scripts with too many atoms.
Comment 2 Brendan Eich [:brendan] 2007-01-10 17:06:10 PST
Comment on attachment 251100 [details] [diff] [review]
Minimal fix v1

>+            if (caseCount + cg->atomList.count - 1 >= JS_BIT(16))
>+                switchOp = JSOP_CONDSWITCH;

Nit: caseCount + cg->atomList.count > JS_BIT(16) seems simpler to me.

/be
Comment 3 Igor Bukanov 2007-01-11 02:40:13 PST
Created attachment 251162 [details] [diff] [review]
Minimal fix v2

Patch to commit with the nit addressed.
Comment 4 Igor Bukanov 2007-01-11 02:46:14 PST
I committed the patch from comment 3 to the trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.234; previous revision: 3.233
done
Comment 5 Brendan Eich [:brendan] 2007-01-11 10:39:13 PST
Comment on attachment 251162 [details] [diff] [review]
Minimal fix v2

>+            /*
>+             * Lookup switch supports only atom indexes bellow 64K limit.

Typo: "below" misspelled.

r=me with that fixed.

/be
Comment 6 Jay Patel [:jay] 2007-01-16 14:33:27 PST
Comment on attachment 251162 [details] [diff] [review]
Minimal fix v2

Approved for both branches, a=jay for drivers.
Comment 7 Igor Bukanov 2007-01-16 15:05:56 PST
Created attachment 251697 [details] [diff] [review]
Minimal fix v2b

Patch to commit to branches with the misspelling in the comment fixed.
Comment 8 Igor Bukanov 2007-01-16 15:08:09 PST
I committed the patch from comment 7 to MOZILLA_1_8_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.66; previous revision: 3.128.2.65
done
Comment 9 Igor Bukanov 2007-01-16 15:11:52 PST
I committed the patch from comment 7 to MOZILLA_1_8_0_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.3.2.17; previous revision: 3.128.2.3.2.16
done
Comment 10 Bob Clary [:bc:] 2007-01-17 04:06:59 PST
Created attachment 251763 [details]
js1_5/Regress/regress-366601.js
Comment 11 Bob Clary [:bc:] 2007-01-29 08:55:54 PST
verified fixed 1.8.0, 1.8.1, 1.9.0 2007-01-23 win/mac*/linux
Comment 12 Bob Clary [:bc:] 2007-08-08 07:54:18 PDT
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-366601.js,v  <--  regress-366601.js
initial revision: 1.1

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