Last Comment Bug 412926 - JS_ValueToId(cx, JSVAL_NULL) should return atom for 'null' string
: JS_ValueToId(cx, JSVAL_NULL) should return atom for 'null' string
Status: VERIFIED FIXED
[sg:critical?] no sg:critical crash i...
: verified1.8.1.13
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-18 03:08 PST by Igor Bukanov
Modified: 2008-03-29 16:03 PDT (History)
5 users (show)
mtschrep: blocking1.9+
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next-
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix for 1.8.1 (795 bytes, patch)
2008-02-15 13:01 PST, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.13+
Details | Diff | Splinter Review
js1_5/extensions/regress-412926.js (3.07 KB, text/plain)
2008-02-22 04:28 PST, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2008-01-18 03:08:45 PST
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.
Comment 1 Igor Bukanov 2008-01-18 03:21:32 PST
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.
Comment 2 Igor Bukanov 2008-01-18 03:33:09 PST
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.
Comment 3 Daniel Veditz [:dveditz] 2008-02-07 14:38:46 PST
If bug 412340 is marked fixed then is this also fixed on trunk?
Comment 4 Daniel Veditz [:dveditz] 2008-02-15 12:09:24 PST
Can we get a separate branch patch here? Or do we also need bug 412340 on the 1.8 branch?
Comment 5 Igor Bukanov 2008-02-15 12:45:10 PST
(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.
Comment 6 Igor Bukanov 2008-02-15 12:48:46 PST
This has been fixed oo the trunk since the resolution of bug 412340n
Comment 7 Igor Bukanov 2008-02-15 13:01:16 PST
Created attachment 303594 [details] [diff] [review]
fix for 1.8.1
Comment 8 Daniel Veditz [:dveditz] 2008-02-20 11:32:47 PST
Comment on attachment 303594 [details] [diff] [review]
fix for 1.8.1

approved for 1.8.1.13, a=dveditz for release-drivers
Comment 9 Bob Clary [:bc:] 2008-02-22 04:28:38 PST
Created attachment 304960 [details]
js1_5/extensions/regress-412926.js
Comment 11 Bob Clary [:bc:] 2008-02-26 08:30:50 PST
v
Comment 12 Igor Bukanov 2008-02-29 13:15:39 PST
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
Comment 13 Bob Clary [:bc:] 2008-03-17 04:58:15 PDT
v 1.8.1 linux|mac
Comment 14 Alexander Sack 2008-03-23 12:34:40 PDT
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.
Comment 15 Bob Clary [:bc:] 2008-03-29 16:03:19 PDT
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-412926.js,v  <--  regress-412926.js
initial revision: 1.1

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