Closed
Bug 777788
Opened 12 years ago
Closed 12 years ago
IonMonkey: JM to Ion calls are not monitoring the returned value when required by the Type Inference.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox-esr10 | --- | unaffected |
People
(Reporter: nbp, Assigned: dvander)
References
Details
(Keywords: sec-moderate, Whiteboard: [ion:t])
Attachments
(4 files, 5 obsolete files)
21.69 KB,
patch
|
Details | Diff | Splinter Review | |
1.41 KB,
application/x-javascript
|
Details | |
31.52 KB,
text/plain
|
Details | |
11.09 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
The fix to Bug 777537 which sanitize the current usage of the CompilerOutputs highlight and/or cause invalid reads and errors. The current implementation has dandling pointers to previously used compilation outputs. These pointers are not supposed to be used unless checked against the current JSScript content which guarantee their validity.
Reporter | ||
Comment 1•12 years ago
|
||
Test case highlighting the issue.
Reporter | ||
Comment 2•12 years ago
|
||
Valgrind output created by running --smc-check=all
Assignee | ||
Updated•12 years ago
|
Keywords: sec-critical → sec-moderate
Whiteboard: [js:t]
Assignee | ||
Updated•12 years ago
|
Whiteboard: [js:t] → [ion:t]
Reporter | ||
Comment 3•12 years ago
|
||
Update patch.
Reporter | ||
Comment 4•12 years ago
|
||
Another reduction of the same test case.
Attachment #646196 -
Attachment is obsolete: true
Attachment #646197 -
Attachment is obsolete: true
Attachment #646198 -
Attachment is obsolete: true
Reporter | ||
Comment 5•12 years ago
|
||
Almost readable type error message.
Reporter | ||
Comment 6•12 years ago
|
||
The main function of the « reduced » test case is not compiled with --ion-eager, This cause the main function to be compiled with JM once all smaller functions are compiled with IonMonkey. Type inference mention that the result of the call should be monitored. My guess is that we are missing a monitored call and return to the interpreter when executing the « expected[i] » which cause the script to get back into the interpreter which then fails when try to call the function with « a » which does not contain any monitored type.
Summary: IonMonkey: TI: Using a uniq Compiler Output cause unexpected failures. → IonMonkey: JM to Ion calls are not monitoring the returned value when required by the Type Inference.
Reporter | ||
Comment 7•12 years ago
|
||
Provide arguments to back up the previous comment.
Attachment #648241 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
Yup, you're absolutely right, thanks for the analysis. Originally the JM->Ion patch bounced back to an earlier rejoin point, but now it uses the native one and natives must monitor their result.
Assignee: general → dvander
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•12 years ago
|
||
Nicolas, could you see if this fixes your problem? It's not 100% working yet (math-jit-tests fails) but it's getting close. I think the remaining problem is that invalidation now is missing a TypeMonitor, rather than the inline path. If that's the case I may abandon this approach and perform the type monitor inside the IC.
Assignee | ||
Comment 10•12 years ago
|
||
This patch makes JM->Ion ICs join right after the normal inline call return site, in between fp->prev being popped and the return type barrier.
Attachment #648930 -
Attachment is obsolete: true
Attachment #649802 -
Flags: review?(jdemooij)
Comment 11•12 years ago
|
||
Comment on attachment 649802 [details] [diff] [review] fix Review of attachment 649802 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/testPos.js @@ +30,2 @@ > assertEq(f_double("-2.147483648"), -2.147483648); > + // assertEq(f_double("2.147483647"), 2.147483647); Please revert the changes to this file.
Attachment #649802 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/c1f46fa5a67d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•