313938 - Unrooted access in jsscript.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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]

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]

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]

Attachment: Fix

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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]

(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) (inactive)]

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]

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 emeritus)]

Comment on attachment 201107 [details] [diff] [review]
Fix

sr=shaver

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 201107 [details] [diff] [review]
Fix

This is safe for 1.8 checkin at any time.

/be

Comment 10

[Brendan Eich [:brendan]]

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]]

Though blocking, please wait for patch approval to land.

Comment 12

[Brendan Eich [:brendan]]

Attachment: roll-up patch to ride along

Comment 13

[Scott MacGregor]

moving this to the nomination state with the rest of our RC2 ride along bugs. 

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/String/regress-313938.js

Comment 15

[Asa Dotzler [:asa]]

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]]

Fixed on the 1.8 branch.

/be

Comment 17

[Bob Clary [:bc] (inactive)]

verified firefox 1.5 rc2 linux/win32 2005-11-07

Comment 18

[Bob Clary [:bc] (inactive)]

testcase+ to get this off my radar. when this is made public, i will check in
the test.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Fixes checked into the 1.7 branch.

Comment 20

[Bob Clary [:bc] (inactive)]

verified firefox/1.7.13,1.8.0.1,1.8,1.9a1 win/linux/mac

Comment 21

[Bob Clary [:bc] (inactive)]

v moz 1.7.13 windows 20060212

Comment 22

[Bob Clary [:bc] (inactive)]

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