Closed Bug 313938 Opened 16 years ago Closed 16 years ago

Unrooted access in jsscript.c

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

Details

(Keywords: js1.6, verified1.7.13, verified1.8, Whiteboard: [sg:critical?])

Attachments

(3 files)

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);
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.
Summary: Unrooted access in jsscript.c → Unrooted access in jsscript.c
Flags: blocking1.8rc1?
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
Whiteboard: [sg:critical?]
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);
Assignee: general → igor.bukanov
Attached patch FixSplinter Review
Attachment #201107 - Flags: superreview?(shaver)
Attachment #201107 - Flags: review?(brendan)
Comment on attachment 201107 [details] [diff] [review]
Fix

I think we should get these in as quickly as possible. r=mrbkap
Attachment #201107 - Flags: review+
(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?
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.
I committed the fix to the trunk: marking as fixed. Somebody with sufficient power can change those "?" in bloking nomintaions to "+".
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment on attachment 201107 [details] [diff] [review]
Fix

sr=shaver
Attachment #201107 - Flags: superreview?(shaver) → superreview+
Comment on attachment 201107 [details] [diff] [review]
Fix

This is safe for 1.8 checkin at any time.

/be
Attachment #201107 - Flags: review?(brendan)
Attachment #201107 - Flags: review+
Attachment #201107 - Flags: approval1.8rc1?
Script thaw also fails to set a converted argv.  I'll put a branch patch here that fixes both, in a minute.

/be
Though blocking, please wait for patch approval to land.
Flags: blocking1.8rc1?
Flags: blocking1.8rc1+
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
Attachment #201179 - Flags: superreview+
Attachment #201179 - Flags: review+
Attachment #201179 - Flags: approval1.8rc1?
Attachment #201107 - Flags: approval1.8rc1?
moving this to the nomination state with the rest of our RC2 ride along bugs. 
Flags: blocking1.8rc1+ → blocking1.8rc1?
Flags: testcase?
moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.
Flags: blocking1.8rc1? → blocking1.8rc2?
Attachment #201179 - Flags: approval1.8rc1? → approval1.8rc1+
Attachment #201179 - Flags: approval1.8rc1+ → approval1.8rc2+
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8, js1.6
Flags: blocking1.8rc2?
verified firefox 1.5 rc2 linux/win32 2005-11-07
Keywords: fixed1.8verified1.8
testcase+ to get this off my radar. when this is made public, i will check in
the test.
Flags: testcase? → testcase+
Fixes checked into the 1.7 branch.
verified firefox/1.7.13,1.8.0.1,1.8,1.9a1 win/linux/mac
Status: RESOLVED → VERIFIED
v moz 1.7.13 windows 20060212
Group: security
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
You need to log in before you can comment on or make changes to this bug.