Unrooted access in jsscript.c

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({js1.6, verified1.7.13, verified1.8})

Trunk
x86
Linux
js1.6, verified1.7.13, verified1.8
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(3 attachments)

(Assignee)

Description

12 years ago
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);
(Assignee)

Comment 1

12 years ago
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?]
(Assignee)

Comment 2

12 years ago
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
(Assignee)

Comment 3

12 years ago
Created attachment 201107 [details] [diff] [review]
Fix
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+
(Assignee)

Comment 5

12 years ago
(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.
(Assignee)

Comment 7

12 years ago
I committed the fix to the trunk: marking as fixed. Somebody with sufficient power can change those "?" in bloking nomintaions to "+".
Status: NEW → RESOLVED
Last Resolved: 12 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+
Created attachment 201179 [details] [diff] [review]
roll-up patch to ride along
Attachment #201179 - Flags: superreview+
Attachment #201179 - Flags: review+
Attachment #201179 - Flags: approval1.8rc1?

Updated

12 years ago
Attachment #201107 - Flags: approval1.8rc1?

Comment 13

12 years ago
moving this to the nomination state with the rest of our RC2 ride along bugs. 
Flags: blocking1.8rc1+ → blocking1.8rc1?

Comment 14

12 years ago
Created attachment 201231 [details]
js1_5/String/regress-313938.js

Updated

12 years ago
Flags: testcase?

Comment 15

12 years ago
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?

Updated

12 years ago
Attachment #201179 - Flags: approval1.8rc1? → approval1.8rc1+

Updated

12 years ago
Attachment #201179 - Flags: approval1.8rc1+ → approval1.8rc2+
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8, js1.6

Updated

12 years ago
Flags: blocking1.8rc2?

Comment 17

12 years ago
verified firefox 1.5 rc2 linux/win32 2005-11-07
Keywords: fixed1.8 → verified1.8

Comment 18

12 years ago
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.
Keywords: fixed-aviary1.0.8, fixed1.7.13

Comment 20

12 years ago
verified firefox/1.7.13,1.8.0.1,1.8,1.9a1 win/linux/mac
Status: RESOLVED → VERIFIED
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8

Comment 21

12 years ago
v moz 1.7.13 windows 20060212
Keywords: fixed1.7.13 → verified1.7.13
Group: security

Comment 22

11 years ago
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.