Closed
Bug 366601
Opened 18 years ago
Closed 18 years ago
switch assumes all atom indexes below 64K
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: igor, Assigned: igor)
Details
(Keywords: verified1.8.0.10, verified1.8.1.2, Whiteboard: [sg:nse] hints at similar bugs 365692, 366122)
Attachments
(2 files, 2 obsolete files)
1.28 KB,
patch
|
Details | Diff | Splinter Review | |
2.59 KB,
text/plain
|
Details |
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.
Assignee | ||
Comment 1•18 years ago
|
||
A minimal fix to use a conditional switch for scripts with too many atoms.
Attachment #251100 -
Flags: review?(brendan)
Comment 2•18 years ago
|
||
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
Attachment #251100 -
Flags: review?(brendan)
Attachment #251100 -
Flags: review+
Attachment #251100 -
Flags: approval1.8.1.2?
Attachment #251100 -
Flags: approval1.8.0.10?
Assignee | ||
Comment 3•18 years ago
|
||
Patch to commit with the nit addressed.
Attachment #251100 -
Attachment is obsolete: true
Attachment #251162 -
Flags: review+
Attachment #251162 -
Flags: approval1.8.1.2?
Attachment #251162 -
Flags: approval1.8.0.10?
Attachment #251100 -
Flags: approval1.8.1.2?
Attachment #251100 -
Flags: approval1.8.0.10?
Assignee | ||
Updated•18 years ago
|
Attachment #251162 -
Attachment description: Minimal fix v1 → Minimal fix v2
Assignee | ||
Comment 4•18 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 5•18 years ago
|
||
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•18 years ago
|
||
Comment on attachment 251162 [details] [diff] [review] Minimal fix v2 Approved for both branches, a=jay for drivers.
Attachment #251162 -
Flags: approval1.8.1.2?
Attachment #251162 -
Flags: approval1.8.1.2+
Attachment #251162 -
Flags: approval1.8.0.10?
Attachment #251162 -
Flags: approval1.8.0.10+
Updated•18 years ago
|
Whiteboard: [sg:nse] hints at similar bugs 365692, 366122
Assignee | ||
Comment 7•18 years ago
|
||
Patch to commit to branches with the misspelling in the comment fixed.
Attachment #251162 -
Attachment is obsolete: true
Assignee | ||
Comment 8•18 years ago
|
||
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
Keywords: fixed1.8.1.2
Assignee | ||
Comment 9•18 years ago
|
||
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
Keywords: fixed1.8.0.10
Summary: switch assumes all atom indexes bellow 64K → switch assumes all atom indexes below 64K
Comment 10•18 years ago
|
||
Updated•18 years ago
|
Flags: in-testsuite+
Comment 11•18 years ago
|
||
verified fixed 1.8.0, 1.8.1, 1.9.0 2007-01-23 win/mac*/linux
Status: RESOLVED → VERIFIED
Updated•17 years ago
|
Group: security
Comment 12•17 years ago
|
||
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-366601.js,v <-- regress-366601.js initial revision: 1.1
You need to log in
before you can comment on or make changes to this bug.
Description
•