As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 379245 - Interpreter on 1.8.* does not clear obj register during inline calls
: Interpreter on 1.8.* does not clear obj register during inline calls
Status: VERIFIED FIXED
[sg:moderate?] 1.8-branch only
: fixed1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-30 04:00 PDT by Igor Bukanov
Modified: 2007-09-21 07:33 PDT (History)
3 users (show)
dveditz: blocking1.8.1.5+
dveditz: blocking1.8.0.13+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1 (4.05 KB, patch)
2007-04-30 04:22 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.5+
Details | Diff | Splinter Review
Fix v1 for 1.8.0 branch (1.54 KB, patch)
2007-07-10 06:10 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review
diff between 1.8.1 and 1.8.0 versions of fix v1 (3.39 KB, text/plain)
2007-07-10 06:11 PDT, Igor Bukanov
no flags Details
js1_5/Regress/regress-379245.js (2.42 KB, text/plain)
2007-07-13 10:36 PDT, Bob Clary [:bc:]
no flags Details

Description User image Igor Bukanov 2007-04-30 04:00:39 PDT
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
Comment 1 User image Igor Bukanov 2007-04-30 04:04:59 PDT
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. 
Comment 2 User image Igor Bukanov 2007-04-30 04:22:33 PDT
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.
Comment 3 User image Igor Bukanov 2007-04-30 08:51:35 PDT
The bug does not exist on trunk due to changes from bug 363530.
Comment 4 User image Daniel Veditz [:dveditz] 2007-07-09 11:25:45 PDT
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.
Comment 5 User image Igor Bukanov 2007-07-10 06:03:29 PDT
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
Comment 6 User image Igor Bukanov 2007-07-10 06:09:07 PDT
Comment on attachment 263246 [details] [diff] [review]
Fix v1

The patch requires a merger for 1.8.0 branch.
Comment 7 User image Igor Bukanov 2007-07-10 06:10:26 PDT
Created attachment 271662 [details] [diff] [review]
Fix v1 for 1.8.0 branch
Comment 8 User image Igor Bukanov 2007-07-10 06:11:40 PDT
Created attachment 271663 [details]
diff between 1.8.1 and 1.8.0 versions of fix v1
Comment 9 User image Brendan Eich [:brendan] 2007-07-10 12:01:49 PDT
Comment on attachment 271662 [details] [diff] [review]
Fix v1 for 1.8.0 branch

Are we still maintaining 1.8.0?

/be
Comment 10 User image Igor Bukanov 2007-07-10 12:19:31 PDT
(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 User image Bob Clary [:bc:] 2007-07-13 10:36:52 PDT
Created attachment 272200 [details]
js1_5/Regress/regress-379245.js
Comment 12 User image Bob Clary [:bc:] 2007-07-17 10:47:16 PDT
verified fixed 1.8.1 windows/linux/macppc opt/debug shell/browser 7/16
Comment 13 User image Daniel Veditz [:dveditz] 2007-08-07 10:05:36 PDT
Comment on attachment 271662 [details] [diff] [review]
Fix v1 for 1.8.0 branch

approved for 1.8.0.13, a=dveditz
Comment 14 User image Daniel Veditz [:dveditz] 2007-08-09 11:17:33 PDT
Fix landed for 1.8.0.13
Comment 15 User image Bob Clary [:bc:] 2007-09-21 07:33:50 PDT
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-379245.js,v  <--  regress-379245.js
initial revision: 1.1

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