Last Comment Bug 356238 - Avoid null, dups and bad indexes in jsxml.c
: Avoid null, dups and bad indexes in jsxml.c
Status: VERIFIED FIXED
[sg:critical]
: verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on: 354145
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-10 18:06 PDT by Igor Bukanov
Modified: 2007-06-15 10:01 PDT (History)
2 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1 (3.17 KB, patch)
2006-10-11 07:14 PDT, Igor Bukanov
brendan: review+
mconnor: approval1.8.0.9+
mconnor: approval1.8.1.1+
Details | Diff | Splinter Review
e4x/Regress/regress-356238-01.js (1.97 KB, text/plain)
2006-10-12 03:24 PDT, Bob Clary [:bc:]
no flags Details
e4x/Regress/regress-356238-02.js (2.22 KB, text/plain)
2006-10-12 03:25 PDT, Bob Clary [:bc:]
no flags Details
e4x/Regress/regress-356238-02.js (2.23 KB, text/plain)
2006-10-12 12:47 PDT, Bob Clary [:bc:]
no flags Details
e4x/Regress/regress-356238-03.js (2.28 KB, text/plain)
2006-10-13 09:19 PDT, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2006-10-10 18:06:22 PDT
[ This is a spin-off of security-sensitive bug 354145 comment 71. ]

With the bug 354145 fixed xml_child_helper may set rval to JSVAL_NULL, but xml_child does not check for this leading to potential null pointer access.
Comment 1 Igor Bukanov 2006-10-11 06:07:15 PDT
I am renaming the bug to reflect its generic nature for extra fixes after landing bug 354145.

  
Comment 2 Igor Bukanov 2006-10-11 06:14:46 PDT
Replace method in jsxml.c contains an assert that checks that the passed index is not exceeding the length. But this is a wrong assert as Replace is called directly from  xml_replace so it can contain an arbitrary index. This assert is harmless since the code resets the index in any case later. The following one-liner shows the problem in a debug build:


js> <xml/>.replace(1);
Assertion failure: i <= n, at jsxml.c:3834
Trace/breakpoint trap (core dumped)
Comment 3 Igor Bukanov 2006-10-11 06:45:20 PDT
Here is a test case that show that Insert can create duplicates: 

var xml = <xml><child><a/><b/></child></xml>;
var child = xml.child[0];

try {
  child.insertChildAfter(null, xml.child);
  throw "insertChildAfter succeded when it should throw an exception";
} catch (e if e instanceof Error) {
}

if (child.a[0] === child.a[1])
  throw "Duplicate detected!"


Currently it throws "Duplicate detected!" since Insert first reserve the room for list elements to insert and then checks for loops there.
Comment 4 Igor Bukanov 2006-10-11 07:14:14 PDT
Created attachment 241938 [details] [diff] [review]
Fix v1
Comment 5 Brendan Eich [:brendan] 2006-10-11 09:34:55 PDT
Comment on attachment 241938 [details] [diff] [review]
Fix v1

Looks good, some questions:

* Does Rhino have this bug?
* Is this a spec bug?
* Why undefined instead of null from XML.prototype.child?  Spec doesn't help here, so suspect a spec bug in part.

/be
Comment 6 Igor Bukanov 2006-10-11 13:59:53 PDT
(In reply to comment #5)
> (From update of attachment 241938 [details] [diff] [review] [edit])
> Looks good, some questions:
> 
> * Does Rhino have this bug?

No. In Rhino E4X implementation is effectively a bridge from XMLBeans Apache library and JS. Thus all the unwrapping of object into corresponding native classes is done very early or not done at all. That is, in Rhino in many places the code just returns when it see NON-xml object. 

> * Is this a spec bug?

[[Insert]] and [[Replace]] definitions, 9.1.1.11 and 9.1.1.12 do not require to detect cycles. In particular [[Insert]] states:

1. If x.[[Class]] ∈ {"text", "comment", "processing-instruction", "attribute"}, return
2. Let i = ToUint32(P)
3. If (ToString(i) is not equal to P), throw a TypeError exception
4. Let n = 1
5. If Type(V) is XMLList, let n = V.[[Length]]
6. If n == 0, Return
7. For j = x.[[Length]]-1 downto i, rename property ToString(j) of x to ToString(j + n)
8. Let x.[[Length]] = x.[[Length]] + n
9. If Type(V) is XMLList
     a. For j = 0 to V.[[Length-1]]
          i. V[j].[[Parent]] = x
         ii. x[i + j] = V[j]
              NOTE: the E4X data model does not enforce the constraint:
      ∀ x ∈ XML : x.[[InScopeNamespaces]] ⊇ x.[[Parent]].[[InScopeNamespaces]]. 
              However, implementations may at this point add namespaces from
              V[j].[[InScopeNamespaces]] to x or any ancestors of x. Likewise,
              implementations may at this point add namespaces from x to V[j]
              or any descendents of V[j].

10. Else
     a. Call the [[Replace]] method of x with arguments i and V
11. Return

> * Why undefined instead of null from XML.prototype.child?  Spec doesn't help
> here, so suspect a spec bug in part.

Because JSVAL_VOID is already used by xml_child_helper to indicate that child does not exist and I wanted a minimal patch. Note that rval in xml_child_helper represent local jsval in xml_child which is never exposed to scripts. 
Comment 7 Brendan Eich [:brendan] 2006-10-11 14:32:45 PDT
(In reply to comment #6)
> > * Is this a spec bug?
> 
> [[Insert]] and [[Replace]] definitions, 9.1.1.11 and 9.1.1.12 do not require to
> detect cycles.

Actually we added cycle checking to Edition 2 of ECMA-357 (what went to ISO), IIRC.  I'll see if I can find the .pdf.

Ok, patch looks good, thanks for answers.  Stamping in a minute.

/be
Comment 8 Brendan Eich [:brendan] 2006-10-11 14:33:32 PDT
Comment on attachment 241938 [details] [diff] [review]
Fix v1

r=me, thanks again.

/be
Comment 9 Brendan Eich [:brendan] 2006-10-11 14:34:29 PDT
Comment on attachment 241938 [details] [diff] [review]
Fix v1

Should this follow the patch for bug 354145 into 1.8.0.x?

/be
Comment 10 Igor Bukanov 2006-10-11 14:44:30 PDT
I committed the patch from comment 4 to the trunk:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.132; previous revision: 3.131
done
Comment 11 Igor Bukanov 2006-10-11 14:45:59 PDT
Comment on attachment 241938 [details] [diff] [review]
Fix v1

The patch applies to 1.8.0 branch as is.
Comment 12 Bob Clary [:bc:] 2006-10-12 03:24:45 PDT
Created attachment 242051 [details]
e4x/Regress/regress-356238-01.js
Comment 13 Bob Clary [:bc:] 2006-10-12 03:25:16 PDT
Created attachment 242052 [details]
e4x/Regress/regress-356238-02.js
Comment 14 Bob Clary [:bc:] 2006-10-12 12:47:27 PDT
Created attachment 242090 [details]
e4x/Regress/regress-356238-02.js

fix the initial value of the actual variable.
Comment 15 Bob Clary [:bc:] 2006-10-12 12:48:01 PDT
verified fixed 1.9 20061012 windows/linux
Comment 16 Igor Bukanov 2006-10-13 09:04:58 PDT
This is not a cleanup or null-pointer-only bug. The problem is that Insert in jsxml.c uses XMLArrayInsert. That in turn calls XMLArraySetCapacity which calls realloc without clearing the extra allocated tail. Then insert in case of tail insert simply increases the array length so junk elements become a part of the array. Since prior this bug fix a script can prevent overwriting them, this allows  for the script to reinterpret as JSXML* a memory that realloc brought, like in the following example:

 @callisto~/m/1.8.1/mozilla/js/src> cat ~/m/files/y2.js 
var xml = <xml><child><a/></child></xml>;
var child = xml.child[0];

try {
  child.insertChildBefore(null, xml.child);
  throw "insertChildBefore succeded when it should throw an exception";
} catch (e if e instanceof Error) {
}

var list = child.*;
var text = uneval(list[1]);
if (text !== "undefined")
  throw "child got unecxpected second element: "+text;
@callisto~/m/1.8.1/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y2.js 
Assertion failure: (uint32)2 < JS_MIN(((obj)->map)->freeslot, ((obj)->map)->nslots), at jsapi.c:2358
Trace/breakpoint trap (core dumped)
@callisto~/m/1.8.1/mozilla/js/src>

On the trunk with the patch fixed the inserted elements are always cleared.
Comment 17 Igor Bukanov 2006-10-13 09:08:50 PDT
(In reply to comment #16)
> On the trunk with the patch fixed the inserted elements are always cleared.

I meant "with the patch landed". That is, the committed fix solved this problem as well.
Comment 18 Bob Clary [:bc:] 2006-10-13 09:19:52 PDT
Created attachment 242196 [details]
e4x/Regress/regress-356238-03.js

additional Igor test from comment 16.
Comment 19 Mike Connor [:mconnor] 2006-11-07 07:28:20 PST
Comment on attachment 241938 [details] [diff] [review]
Fix v1

a=mconnor on behalf of drivers for 1.8.0.9 and 1.8.1.1 checkin
Comment 20 Igor Bukanov 2006-11-07 07:43:15 PST
I committed the patch from comment 4 to MOZILLA_1_8_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.53; previous revision: 3.50.2.52
done
Comment 21 Igor Bukanov 2006-11-07 07:50:41 PST
I committed the patch from comment 4 to MOZILLA_1_8_0_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.15.2.24; previous revision: 3.50.2.15.2.23
done
Comment 22 Bob Clary [:bc:] 2006-11-09 12:38:32 PST
verified fixed 1.8.0.9, 1.8.1.1 20061108 windows/linux/mac*
Comment 23 Igor Bukanov 2006-12-11 05:18:33 PST
> Status Whiteboard: [sg:moderate] (memsnooping?)

See coment 16 where the example crashes due to a reinterpretation of the garbge that fills the tail of realloced memory as JSXML.  
Comment 24 Daniel Veditz [:dveditz] 2006-12-11 09:29:56 PST
Thanks, missed that comment (was triaging a bunch of bugs taking best guess)
Comment 25 Bob Clary [:bc:] 2007-02-08 14:51:49 PST
/cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-01.js,v  <--  regress-356238-01.js

/cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-02.js,v  <--  regress-356238-02.js

/cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-03.js,v  <--  regress-356238-03.js

modified to remove conditional catch clause to remove SpiderMonkey only extension.
Comment 26 Bob Clary [:bc:] 2007-06-15 10:01:36 PDT
bug 375801 changed uneval behavior.

/cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-03.js,v  <--  regress-356238-03.js
new revision: 1.3; previous revision: 1.2

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