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
:
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 | 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 | 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 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 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 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 Igor Bukanov 2007-04-30 08:51:35 PDT
The bug does not exist on trunk due to changes from bug 363530.
Comment 4 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 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 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 Igor Bukanov 2007-07-10 06:10:26 PDT
Created attachment 271662 [details] [diff] [review]
Fix v1 for 1.8.0 branch
Comment 8 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 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 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 Bob Clary [:bc:] 2007-07-13 10:36:52 PDT
Created attachment 272200 [details]
js1_5/Regress/regress-379245.js
Comment 12 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 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 Daniel Veditz [:dveditz] 2007-08-09 11:17:33 PDT
Fix landed for 1.8.0.13
Comment 15 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.