Interpreter on 1.8.* does not clear obj register during inline calls

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.0.13, verified1.8.1.5})

1.8 Branch
fixed1.8.0.13, verified1.8.1.5
Points:
---
Bug Flags:
blocking1.8.1.5 +
blocking1.8.0.13 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate?] 1.8-branch only)

Attachments

(4 attachments)

(Assignee)

Description

10 years ago
Currently the interpreter on 1.8. branches does not clear the object register during when returning from the inlined call. Thus for a case like f()() where f is inlined the interpreter passes as this to the second call the value of obj register left from f() invocation instead of unconditional null as demonstrated by the following example:

var fThis;

function f()
{
   fThis = this;
   return ({x: f}).x;
}

f()();

if (this !== fThis)
  throw "bad this";

Currently when invoked in js shell from 1.8.* branches the code throws the exception:

~/m/1.8.1/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/y.js
uncaught exception: bad this
~/m/1.8.1/mozilla/js/src $ cd ~/m/1.8.0/mozilla/js/src
~/m/1.8.0/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/y.js
uncaught exception: bad this

This is exploitable since a branch callback called when the inlined function returns can trigger GC and the value stored in obj register is not rooted. For example, when the example is run against 1.8.1 shell with branch callback enabled the code crashes during "this" calculations as on 1.8.1 the branch callback calls invokes th GC at the right moment:

~/m/1.8.1/mozilla/js/src $ ./Linux_All_DBG.OBJ/js -b 1000 ~/s/y.js
segmentation fault

The crash does not happen with 1.8.0 shell due to difference in MaybeGC implementation. 




 with enabled branch callback it crashes:

~/m/1.8.1/mozilla/js/src $ ./Linux_All_DBG.OBJ/js -b 1000 ~/s/y.js
segmentation fault

while on 1.8.0 the code gives:

~/m/1.8.0/mozilla/js/src $ ./Linux_All_DBG.OBJ/js -b 1000 ~/s/y.js
uncaught exception: bad this
(Assignee)

Comment 1

10 years ago
My quick attempt to trigger the hazard in a browser using the above example has not succeed in crashing the browser as it is non-trivial to trigger the branch callback that in turn triggers GC at the right moment. But I do not think it is impossible to write deterministic example triggering the crash. 
(Assignee)

Comment 2

10 years ago
Created attachment 263246 [details] [diff] [review]
Fix v1

The fix clear the obj register both on return form the inlined function and on its invocation. The latter is not necessary to fix the bug but I prefer to play safe.
Attachment #263246 - Flags: review?(brendan)
(Assignee)

Comment 3

10 years ago
The bug does not exist on trunk due to changes from bug 363530.

Updated

10 years ago
Attachment #263246 - Flags: review?(brendan) → review+
(Assignee)

Updated

10 years ago
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
(Assignee)

Updated

10 years ago
Attachment #263246 - Flags: approval1.8.0.13?
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Whiteboard: [sg:moderate?] 1.8-branch only
Comment on attachment 263246 [details] [diff] [review]
Fix v1

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers.
Attachment #263246 - Flags: approval1.8.1.5+
Attachment #263246 - Flags: approval1.8.0.13?
Attachment #263246 - Flags: approval1.8.0.13+
(Assignee)

Comment 5

10 years ago
I committed the patch from comment 2 tp MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.90; previous revision: 3.181.2.89
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.5
Resolution: --- → FIXED
(Assignee)

Comment 6

10 years ago
Comment on attachment 263246 [details] [diff] [review]
Fix v1

The patch requires a merger for 1.8.0 branch.
Attachment #263246 - Flags: approval1.8.0.13+
(Assignee)

Comment 7

10 years ago
Created attachment 271662 [details] [diff] [review]
Fix v1 for 1.8.0 branch
Attachment #271662 - Flags: review?(brendan)
Attachment #271662 - Flags: approval1.8.0.13?
(Assignee)

Comment 8

10 years ago
Created attachment 271663 [details]
diff between 1.8.1 and 1.8.0 versions of fix v1
(Assignee)

Updated

10 years ago
Attachment #271663 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 271662 [details] [diff] [review]
Fix v1 for 1.8.0 branch

Are we still maintaining 1.8.0?

/be
Attachment #271662 - Flags: review?(brendan) → review+
(Assignee)

Comment 10

10 years ago
(In reply to comment #9)
> (From update of attachment 271662 [details] [diff] [review])
> Are we still maintaining 1.8.0?

Well, the answer is not, but Linux distros still have to do it. So if the patch is trivial to port, I would like to continue to consider it for 1.8.0.

Comment 11

10 years ago
Created attachment 272200 [details]
js1_5/Regress/regress-379245.js

Updated

10 years ago
Flags: in-testsuite+

Comment 12

10 years ago
verified fixed 1.8.1 windows/linux/macppc opt/debug shell/browser 7/16
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.5 → verified1.8.1.5
Comment on attachment 271662 [details] [diff] [review]
Fix v1 for 1.8.0 branch

approved for 1.8.0.13, a=dveditz
Attachment #271662 - Flags: approval1.8.0.13? → approval1.8.0.13+
Fix landed for 1.8.0.13
Keywords: fixed1.8.0.13
Group: security

Comment 15

10 years ago
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-379245.js,v  <--  regress-379245.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.