Last Comment Bug 313938 - Unrooted access in jsscript.c
: Unrooted access in jsscript.c
Status: VERIFIED FIXED
[sg:critical?]
: js1.6, verified1.7.13, verified1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-26 14:19 PDT by Igor Bukanov
Modified: 2006-06-14 17:11 PDT (History)
4 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (760 bytes, patch)
2005-10-28 01:52 PDT, Igor Bukanov
brendan: review+
mrbkap: review+
shaver: superreview+
Details | Diff | Splinter Review
roll-up patch to ride along (1.37 KB, patch)
2005-10-28 12:36 PDT, Brendan Eich [:brendan]
brendan: review+
brendan: superreview+
asa: approval1.8rc2+
Details | Diff | Splinter Review
js1_5/String/regress-313938.js (2.67 KB, text/plain)
2005-10-28 23:14 PDT, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2005-10-26 14:19:31 PDT
script_compile in jsscript.c does not root str, the result of js_ValueToObject applied to the first argument, before calling js_ValueToObject on the second and using str later. The following "standard" demo shows the bug in the shell/browser.

var str = " 2;".substring(1);
"1".substring(2);
var expected = Script.prototype.compile(str).toSource();

var likeString = { 
	toString: function() {
		var tmp = str;
		str = null;
		return tmp;
	}
};

TWO = 2.0;

var likeObject = {
	valueOf: function() {
		if (typeof gc == "function") 
			gc();
		for (var i = 0; i != 40000; ++i) {
			var tmp = 1e100 * TWO;
		}
		return this;
	}
}

var s = Script.prototype.compile(likeString, likeObject);
var actual = s.toSource();
print(expected === actual);
Comment 1 Igor Bukanov 2005-10-26 14:27:26 PDT
I have found the problem accidentally when I tried to eliminate inherently unsafe js_ValueToString by a safer variant 

js_EnsureStringValue(JSContext *cx, jsval *pv);

where pv is a rooted pointer. I believe such "active" search for bugs when one tries to replace the culprits of the resent bugs by a safer versions is a better way to locate problems then simply looking through the code. In particular, I missed the problem when looked through the file few days ago.
Comment 2 Igor Bukanov 2005-10-28 01:49:08 PDT
Bellow is a test case version that shows the bug even with fix from bug 313952 applied. The new version adds extra  "String(likeString);" to replace "str" that newly introduced lastInvokeResult holds by null before forcing GC.

var str = " 2;".substring(1);
"1".substring(2);
var expected = Script.prototype.compile(str).toSource();

var likeString = { 
	toString: function() {
		var tmp = str;
		str = null;
		return tmp;
	}
};

TWO = 2.0;

var likeObject = {
	valueOf: function() {
		String(likeString); // To remove str from lastresult 
		if (typeof gc == "function") 
			gc();
		for (var i = 0; i != 40000; ++i) {
			var tmp = 1e100 * TWO;
		}
		return this;
	}
}

var s = Script.prototype.compile(likeString, likeObject);
var actual = s.toSource();
print(expected === actual);
Comment 3 Igor Bukanov 2005-10-28 01:52:42 PDT
Created attachment 201107 [details] [diff] [review]
Fix
Comment 4 Blake Kaplan (:mrbkap) 2005-10-28 02:20:18 PDT
Comment on attachment 201107 [details] [diff] [review]
Fix

I think we should get these in as quickly as possible. r=mrbkap
Comment 5 Igor Bukanov 2005-10-28 02:32:21 PDT
(In reply to comment #4)
> (From update of attachment 201107 [details] [diff] [review] [edit])
> I think we should get these in as quickly as possible. r=mrbkap
> 

Given that should I simply commit the patch, mark it as fixed and nominate for all the branches?
Comment 6 Blake Kaplan (:mrbkap) 2005-10-28 02:36:22 PDT
Yes. We're currently in "lock-down" since RC1 is currently out. If there's an RC2 then drivers will go over the nominations list to get bugs approved and into the branch.
Comment 7 Igor Bukanov 2005-10-28 02:57:19 PDT
I committed the fix to the trunk: marking as fixed. Somebody with sufficient power can change those "?" in bloking nomintaions to "+".
Comment 8 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-28 08:38:04 PDT
Comment on attachment 201107 [details] [diff] [review]
Fix

sr=shaver
Comment 9 Brendan Eich [:brendan] 2005-10-28 11:13:11 PDT
Comment on attachment 201107 [details] [diff] [review]
Fix

This is safe for 1.8 checkin at any time.

/be
Comment 10 Brendan Eich [:brendan] 2005-10-28 11:14:26 PDT
Script thaw also fails to set a converted argv.  I'll put a branch patch here that fixes both, in a minute.

/be
Comment 11 Daniel Veditz [:dveditz] 2005-10-28 11:50:59 PDT
Though blocking, please wait for patch approval to land.
Comment 12 Brendan Eich [:brendan] 2005-10-28 12:36:26 PDT
Created attachment 201179 [details] [diff] [review]
roll-up patch to ride along
Comment 13 Scott MacGregor 2005-10-28 16:11:52 PDT
moving this to the nomination state with the rest of our RC2 ride along bugs. 
Comment 14 Bob Clary [:bc:] 2005-10-28 23:14:34 PDT
Created attachment 201231 [details]
js1_5/String/regress-313938.js
Comment 15 Asa Dotzler [:asa] 2005-10-31 14:47:41 PST
moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.
Comment 16 Brendan Eich [:brendan] 2005-10-31 17:24:00 PST
Fixed on the 1.8 branch.

/be
Comment 17 Bob Clary [:bc:] 2005-11-07 22:30:20 PST
verified firefox 1.5 rc2 linux/win32 2005-11-07
Comment 18 Bob Clary [:bc:] 2005-12-27 10:36:38 PST
testcase+ to get this off my radar. when this is made public, i will check in
the test.
Comment 19 Blake Kaplan (:mrbkap) 2006-02-10 17:32:39 PST
Fixes checked into the 1.7 branch.
Comment 20 Bob Clary [:bc:] 2006-02-17 04:04:04 PST
verified firefox/1.7.13,1.8.0.1,1.8,1.9a1 win/linux/mac
Comment 21 Bob Clary [:bc:] 2006-02-22 03:54:54 PST
v moz 1.7.13 windows 20060212
Comment 22 Bob Clary [:bc:] 2006-06-14 17:11:59 PDT
RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-313938.js,v
done
Checking in regress-313938.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-313938.js,v  <--  regress-313938.js
initial revision: 1.1
done

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