383426 - document.normalize() crash in OOM situation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (causes OOM and crash)

Steps to reproduce:
1. Load the testcase, preferably in a build that you're running from a terminal window.
2. Wait 20s for it to load.
3. Wait 100s for it to use 4G of virtual memory.  (Watch it in Activity Monitor and/or watch for console output from malloc.)
4. Click the button.

Result: Crash in which pointers have been overwritten by bytes from UTF-16 text.

(gdb) bt
#0  0x45603003 in ?? ()
#1  0x18085ece in nsGenericElement::JoinTextNodes (this=0x410041, aFirst=0x410041, aSecond=0x410041) at /Users/jruderman/trunk/mozilla/content/base/src/nsGenericElement.cpp:1748
#2  0x00410041 in dyld_stub_fflush ()
Previous frame inner to this frame (corrupt stack?)

I hit this crash while trying to reproduce bug 374244.

Comment 1

[Jesse Ruderman]

Attachment: stack trace to the malloc failure in question

I think the problem is that AppendASCIItoUTF16 doesn't check the return value of aDest.SetLength.  The stack is intact just before the call to copy_string, but once that call finishes, the stack is corrupted.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/string/src/nsReadableUtils.cpp&rev=1.68&mark=137,149#137

Comment 2

[Jesse Ruderman]

Attachment: patch to AppendASCIItoUTF16

This fixes the crash during normalize() for me.  I get a crash later (in layout rather than DOM), due to the "new" operator thinking it should throw rather than return null, but that's much better than a corrupted stack :)

If this is the right approach, I can write a patch that does the same thing to all the "Append" functions in nsReadableUtils.cpp.

Comment 3

[Peter Van der Beken [:peterv]]

You could use GetMutableData (see also bug 335957).

Comment 4

[Jesse Ruderman]

The GetMutableData interface confuses me.  Does the fact that it returns a length rather than a pointer mean I'm supposed to check that the returned length is correct?  If newLen is 0, how am I supposed to use the return value?

Comment 5

[Peter Van der Beken [:peterv]]

Can't you just return if the length you want is bigger than what GetMutableData returns? That should be able to deal with 0.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jesse, are you going to revise your patch or do you want me to take it over?

Comment 7

[Jesse Ruderman]

I can revise it in a few days, but it would be great if you'd take it over.

Comment 8

[Jesse Ruderman]

Attachment: patch to AppendASCIItoUTF16 using GetMutableData

Like this?

This patch has the same effect for me as the last one (no crash during normalize(), safe crash much later).

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That looks good to me, although I'd fuse these into one statement:

+    PRUint32 actual_new_dest_length;
+
+    actual_new_dest_length = aDest.GetMutableData(&dummy, new_dest_length);

Comment 10

[Jesse Ruderman]

Ok.  I initially had those as separate lines to avoid an 85-character line, but now I see that 90-character lines are not unusual in this file.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please tick to 80 columns, but you could do:

PRUint32 actual_new_dest_length
  = aDest.GetMutableData(&dummy, new_dest_length);

Comment 12

[Jesse Ruderman]

Should I make a patch that does the same thing to all the Append conversion functions?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds like a great idea.

Comment 14

[Jesse Ruderman]

Attachment: patch to fix all uses of SetLength in nsReadableUtils.cpp

This introduces a "SetLengthForWriting" function so I don't have to include a bunch of redundant code in each Append* function.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 268904 [details] [diff] [review]
patch to fix all uses of SetLength in nsReadableUtils.cpp

+bool

PRBool

+    if (!SetLengthForWritingC(aDest, old_dest_length + aSource.Length()))
+      {
+        return;
+      }

Better to just write:

+    if (!SetLengthForWritingC(aDest, old_dest_length + aSource.Length()))
+        return;

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 268904 [details] [diff] [review]
patch to fix all uses of SetLength in nsReadableUtils.cpp

Ideally we'd also use the pointer returned by GetMutableData instead of using iterators, but let's just fix the crash first.

Comment 17

[Jesse Ruderman]

Attachment: patch with review comments addressed

Comment 18

[Jesse Ruderman]

Fixed on trunk.  This testcase is way too slow for an automated test.  Using ulimit (Linux only?) could make it faster, or maybe we could make a C++ test file that uses the string classes directly.

Comment 19

[Jesse Ruderman]

Loading the testcase and waiting 10 minutes makes my branch build abort:

firefox-bin(5906,0xa000d000) malloc: *** vm_allocate(size=33558528) failed (error code=3)
firefox-bin(5906,0xa000d000) malloc: *** error: can't allocate region
firefox-bin(5906,0xa000d000) malloc: *** set a breakpoint in szone_error to debug
terminate called after throwing an instance of 'std::bad_alloc'
  what():  St9bad_alloc

That makes it hard to use the testcase to determine whether branch has the memory safety bug, since I don't get a chance to click the normalize button.