JS_ValueToId(cx, JSVAL_NULL) should return atom for 'null' string

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.1.13})

Trunk
verified1.8.1.13
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.13 +
wanted1.8.1.x +
blocking1.8.0.next -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] no sg:critical crash in 1.8.1, fixed by 412340 on trunk,)

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
Consider the following test case:

var obj = { 'null': 1 };

var errors = [];
if (!obj.hasOwnProperty(null))
    errors.push('null property is not owned');
if (!obj.propertyIsEnumerable(null))
    errors.push('null property is not enumerable');

var getter_was_called = false;
obj.__defineGetter__(null, function() { getter_was_called = true; return 1; });
obj['null'];
if (!getter_was_called)
    errors.push('getter was not assigned to the null property')

var setter_was_called = false;
obj.__defineSetter__(null, function() { setter_was_called = true; });
obj['null'] = 2;
if (!setter_was_called)
    errors.push('setter was not assigned to the null property')

if (errors.length)
    throw '\n'+errors.join('\n');

gc();


When I run the test case with the shell I got:
~/m/trunk/mozilla/js/src $ js ~/m/y.js
uncaught exception: 
null property is not owned
null property is not enumerable
getter was not assigned to the null property
setter was not assigned to the null property
segmentation fault

The reason for the exceptions (which also exists on 1.8.1 branch) is that the test case triggers invocation of JS_ValueToId with the id set to null. Currently the function will return it as-is due to the JSVAL_IS_OBJECT check done for XML support when the expected behavior is to return the id for 'null' atom. Hence the test cases will not find the id when looking for the property.

The reason for the segmentation fault (which does *not* exist on 1.8.1 branch) is uncler. In principle the code should tolerate the scope properties with null as an id, but apparently this is not the case. So I mark the bug as sensitive one until I figure out the reason behind the crash.
(Assignee)

Comment 1

10 years ago
The reason for the segmentation fault is that jsscope.c uses JSVAL_NULL as a special value for the id of the scope to indicate that the scope belongs to the free list. This is also the case on the branch but so far I was not able to trigger the fault.
(Assignee)

Comment 2

10 years ago
I have added the fix to the patch from bug 412340 comment 31. This is the only change between v4 and v5 patches in that bug. Since I can not prove that the bug can not be used for an exploit on 1.8.1 branch, I prefer to embed the fix into a bigger patch to avoid exposure.
Flags: blocking1.9?
Flags: blocking1.8.1.13?

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
If bug 412340 is marked fixed then is this also fixed on trunk?
Flags: wanted1.8.1.x+
Whiteboard: [sg:critical?]
Can we get a separate branch patch here? Or do we also need bug 412340 on the 1.8 branch?
Flags: blocking1.8.1.13? → blocking1.8.1.13+
(Assignee)

Comment 5

10 years ago
(In reply to comment #4)
> Can we get a separate branch patch here? Or do we also need bug 412340 on the
> 1.8 branch?
> 

It is unsafe to port 412340 for the branch. So I will create a small targeted fix.
(Assignee)

Comment 6

10 years ago
This has been fixed oo the trunk since the resolution of bug 412340n
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

10 years ago
Created attachment 303594 [details] [diff] [review]
fix for 1.8.1
Attachment #303594 - Flags: review?(brendan)

Updated

10 years ago
Attachment #303594 - Flags: review?(brendan)
Attachment #303594 - Flags: review+
Attachment #303594 - Flags: approval1.8.1.13?
Whiteboard: [sg:critical?] → [sg:critical?] no sg:critical crash in 1.8.1, fixed by 412340 on trunk,
Comment on attachment 303594 [details] [diff] [review]
fix for 1.8.1

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #303594 - Flags: approval1.8.1.13? → approval1.8.1.13+

Comment 9

10 years ago
Created attachment 304960 [details]
js1_5/extensions/regress-412926.js

Updated

10 years ago
Flags: in-testsuite+
Flags: in-litmus-

Comment 11

10 years ago
v
Status: RESOLVED → VERIFIED
(Assignee)

Comment 12

10 years ago
I checked in the patch from comment 7 to MOZILLA_1_8_BRANCH:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.214.2.41; previous revision: 3.214.2.40
done
Keywords: fixed1.8.1.13

Comment 13

9 years ago
v 1.8.1 linux|mac
Keywords: fixed1.8.1.13 → verified1.8.1.13

Comment 14

9 years ago
the code touched in the 1.8.1 patch was introduced by js1.7 landing. Thus, I assume that this doesn't apply to 1.8.0 branch. If thats wrong, please request blocking1.8.0.15 again.
Flags: blocking1.8.0.15-
Group: security

Comment 15

9 years ago
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-412926.js,v  <--  regress-412926.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.