The default bug view has changed. See this FAQ.

Avoid null, dups and bad indexes in jsxml.c

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.9, verified1.8.1.1})

Trunk
verified1.8.0.9, verified1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

11 years ago
I am renaming the bug to reflect its generic nature for extra fixes after landing bug 354145.

  
Summary: xml_child should check for null rval set by xml_child_helper → Avoid null, dups and bad indexes in jsxml.c
(Assignee)

Comment 2

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

Comment 3

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

Updated

11 years ago
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
(Assignee)

Comment 4

11 years ago
Created attachment 241938 [details] [diff] [review]
Fix v1
Attachment #241938 - Flags: review?(brendan)
Attachment #241938 - Flags: approval1.8.1.1?
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
(Assignee)

Comment 6

11 years ago
(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. 
(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 on attachment 241938 [details] [diff] [review]
Fix v1

r=me, thanks again.

/be
Attachment #241938 - Flags: review?(brendan) → review+
Comment on attachment 241938 [details] [diff] [review]
Fix v1

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

/be
(Assignee)

Comment 10

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

11 years ago
Comment on attachment 241938 [details] [diff] [review]
Fix v1

The patch applies to 1.8.0 branch as is.
Attachment #241938 - Flags: approval1.8.0.9?

Comment 12

11 years ago
Created attachment 242051 [details]
e4x/Regress/regress-356238-01.js

Comment 13

11 years ago
Created attachment 242052 [details]
e4x/Regress/regress-356238-02.js

Updated

11 years ago
Flags: in-testsuite+

Comment 14

11 years ago
Created attachment 242090 [details]
e4x/Regress/regress-356238-02.js

fix the initial value of the actual variable.
Attachment #242052 - Attachment is obsolete: true

Comment 15

11 years ago
verified fixed 1.9 20061012 windows/linux
Status: RESOLVED → VERIFIED
(Assignee)

Comment 16

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

Comment 17

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

11 years ago
Created attachment 242196 [details]
e4x/Regress/regress-356238-03.js

additional Igor test from comment 16.
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
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
Attachment #241938 - Flags: approval1.8.1.1?
Attachment #241938 - Flags: approval1.8.1.1+
Attachment #241938 - Flags: approval1.8.0.9?
Attachment #241938 - Flags: approval1.8.0.9+
(Assignee)

Comment 20

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

Updated

11 years ago
Keywords: fixed1.8.1.1
(Assignee)

Comment 21

11 years ago
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
Keywords: fixed1.8.0.9

Comment 22

11 years ago
verified fixed 1.8.0.9, 1.8.1.1 20061108 windows/linux/mac*
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Whiteboard: [sg:moderate] (mem snooping?)
(Assignee)

Comment 23

10 years ago
> 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.  
Thanks, missed that comment (was triaging a bunch of bugs taking best guess)
Whiteboard: [sg:moderate] (mem snooping?) → [sg:critical]
Group: security

Comment 25

10 years ago
/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

10 years ago
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
You need to log in before you can comment on or make changes to this bug.