Closed Bug 610026 Opened 10 years ago Closed 10 years ago

JSOP_BLOCKCHAIN processing does not handle JSOP_INDEXBASE* ops


(Core :: JavaScript Engine, defect, P1)






(Reporter: brendan, Assigned: brendan)



(Keywords: regression, Whiteboard: [sg:critical?] fixed-in-tracemonkey)


(2 files, 3 obsolete files)

Large programs are possible (need to fix a JSParseNode recycling leak first to get the demo to work). Maybe be s-s, marking that way for now.

This is a regression from the patch for bug 535912.

Attached patch fix, with test (obsolete) — Splinter Review
The test fails without this patch as follows in a release build:

./js1_8_5/regress/regress-610026.js:60: Error: Assertion failed: got (new ReferenceError("x is not defined", "./js1_8_5/regress/regress-610026.js", 55)), expected 42

In an unpatched debug shell it fails like so:

Assertion failure: invalid opcode for fast block chain access, at ../jsinterp.cpp:271

because of the unexpected JSOP_RESETBASE.

We should really simplify our bytecode to use a single immediate format for short and long jumps, regular and "big" literal indexes, etc. It might not even slow the interpreter down. Followup fodder.

Attachment #489040 - Flags: review?(wmccloskey)
This deals with GetBlockChain in a different way.
Thanks Bill -- I will use that GetBlockChain approach (tried it but wanted to kill the too-many-overcomplicated-immediate-encodings bird, will defer that to another bug), merged with other changes and tests. The jsparse.cpp change is for yet another bug.

Attached patch fix, v2 (obsolete) — Splinter Review
Attachment #489040 - Attachment is obsolete: true
Attachment #489073 - Flags: review?
Attachment #489040 - Flags: review?(wmccloskey)
Attachment #489073 - Flags: review? → review?(wmccloskey)
Comment on attachment 489073 [details] [diff] [review]
fix, v2

Great, thanks. It might be good to add a comment to js_AdvanceOverBlockchain along the lines of "watch out for INDEXBASE instructions".
Attachment #489073 - Flags: review?(wmccloskey) → review+
Blocks: 535912
Keywords: regression
Whiteboard: [sg:critical?]
Attached patch patch to commit (obsolete) — Splinter Review
I renamed js_AdvanceOverBlockchain js::AdvanceOverBlockchainOp (the second lowercase-c in "Blockchain" is better with "Op" after to clarify this refers to JSOP_*BLOCKCHAIN; normal CamelCaps spelling would want BlockChain but that seemed less obviously about the two bytecodes).

Attachment #489073 - Attachment is obsolete: true
Attachment #489276 - Flags: review+
Zapped a stray print, and commented the magic numbers.

Attachment #489276 - Attachment is obsolete: true
Attachment #489323 - Flags: review+

Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 656381
Group: core-security
You need to log in before you can comment on or make changes to this bug.