Crash [@ nsAString_internal::Replace] setting innerHTML in a deeply nested context

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: jruderman, Assigned: jruderman)

Tracking

({crash, regression, testcase})

Trunk
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature, )

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

11 years ago
Posted file reduced testcase
Loading http://stuffthathappens.com/blog/2008/03/05/simplicity/ makes Firefox crash immediately.  I got that URL from bug 427083.

The reduced testcase usually causes a crash within a few seconds, between when it says "400" and when it says "500".

If you have MallocScribble turned on, you'll see a bunch of <XXX>, where X is the Chinese character that happens to be U+5555.  This indicates that the string contents (but not the string length) are part of deleted memory.

I think what's going on here is a result of nsContentUtils::CreateContextualFragment using nsAutoTArray<nsAutoString>:

* nsAutoTArray uses memcpy when it expands.
* nsAutoString contains an internal pointer to its built-in buffer.
* Moving using memcpy causes internal pointers to point to the wrong place.

If so, this is a regression from bug 403549.  A 2007-11-12 nightly does not crash while a 2007-11-14 nightly does, which seems to support this hypothesis.

bp-6abbd978-05eb-11dd-b337-0013211cbf8a
Flags: blocking1.9?
Assignee

Comment 1

11 years ago
Assignee

Comment 2

11 years ago
Is changing nsParser::ParseFragment to take an array of nsString instead of an array of nsAutoString a reasonable solution?
Assignee

Updated

11 years ago
Whiteboard: [sg:critical]
Summary: Crash setting innerHTML in a deeply nested context → Crash [@ nsAString_internal::Replace] setting innerHTML in a deeply nested context
Aw snap! Using nsString would work, but would churn more memory.

A better solution would be to count the length of the parent chain and call array.SetCapacity(length) before adding anything to the array.

And also add a BIG comment saying that usually putting nsAutoStrings inside an nsTArray is unsafe, but since we do the SetCapacity trick it's ok in this instance. We wouldn't want people to repeat my mistake here.

Jesse, wanna have a go at it since you already did all the work of debugging?
Assignee: nobody → jruderman
Flags: blocking1.9? → blocking1.9+
Assignee

Comment 4

11 years ago
Given the number of callers of nsParser::ParseFragment, I think it would be saner to switch to nsString instead of trying to make all the callers count correctly.
Agreed. I didn't realize that we had more than one caller of this.
Assignee

Comment 6

11 years ago
Posted patch patch: switch to nsString (obsolete) — Splinter Review
This fixes the bug for me.
Assignee

Updated

11 years ago
Attachment #314770 - Flags: superreview?(jonas)
Attachment #314770 - Flags: review?(jonas)
Assignee

Comment 7

11 years ago
Posted patch patch v2Splinter Review
I missed a caller in the last patch, and noticed when I did a full-tree build.
Attachment #314770 - Attachment is obsolete: true
Attachment #314777 - Flags: superreview?(jonas)
Attachment #314777 - Flags: review?(jonas)
Attachment #314770 - Flags: superreview?(jonas)
Attachment #314770 - Flags: review?(jonas)
Comment on attachment 314777 [details] [diff] [review]
patch v2

Would be great if you could fix the other places in the tree that does the same thing too:

http://lxr.mozilla.org/mozilla/search?string=TArray%253CnsAutoString
Attachment #314777 - Flags: superreview?(jonas)
Attachment #314777 - Flags: superreview+
Attachment #314777 - Flags: review?(jonas)
Attachment #314777 - Flags: review+
Assignee

Comment 9

11 years ago
The other places are all in gfx/thebes.  I filed bug 428447 for that.
Assignee

Updated

11 years ago
Attachment #314777 - Flags: approval1.9?
Comment on attachment 314777 [details] [diff] [review]
patch v2

a1.9=beltzner
Attachment #314777 - Flags: approval1.9? → approval1.9+
Assignee

Comment 11

11 years ago
Fixed.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee

Updated

11 years ago
Blocks: 428465
Crash Signature: [@ nsAString_internal::Replace]
Group: core-security
You need to log in before you can comment on or make changes to this bug.