530598 - poison allocator fails when executed under Valgrind

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Johan Björk]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_8; en-us) AppleWebKit/531.9 (KHTML, like Gecko) Version/4.0.3 Safari/531.9
Build Identifier: mozilla-central, bcb580c05f4c+ tip 

Apologies in advance if this is the wrong component, very unfamiliar with the mozilla codebase.

###!!! ABORT: no usable poison region identified: file /Users/phb/Desktop/thesis/code/mozilla-central/layout/base/nsPresArena.cpp, line 212

The cause is that the poison allocator tries to map a region at 0xF0000000, which is where valgrind stores itself.
Changing the poison code to try and map from 0xc0000000 and it works as expected.


Reproducible: Always

Steps to Reproduce:
1. Run under valgrind (even --tool=none reproduces)
2. Observe crash
3.
Actual Results:  
Abort in nsPresArena.cpp:212.


Expected Results:  
Mapping should have succeeded.

Comment 1

[Robert Sayre]

confirming, I just saw this yesterday

Comment 2

[Zack Weinberg (:zwol)]

gah, wrong cc

Comment 3

[Zack Weinberg (:zwol)]

Attachment: possible fix

I can't reproduce the problem (likely it does not happen under 64-bit Linux, regardless of whether the program under test is compiled with 64-bit pointers) but this should fix it.  If you can see the problem, please give it a try.

Heads up chofmann: with this patch in place, we'll take *any* address for the poison region rather than crash on startup.  Crash stats should not be significantly affected, since you have to work at it to get the preferred address not to be chosen, but is there a way to inject application annotations into minidumps?

Comment 4

[Zack Weinberg (:zwol)]

nom'ing for 1.9.2 as a regression from 522088.

Comment 5

[Zack Weinberg (:zwol)]

*sigh* this time for sure.  sorry about the bugspam, folks.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The patch is empty.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: possible fix (for real)

Doh.  Forgot the 'hg qref'.

Comment 8

[Johan Björk]

Tested and it now works with 32bit Valgrind on OSX.

Comment 9

[chris hofmann]

sounds like this fix will repair the 3.6b4 new crash detected in  Bug 531305 that is on the top crash list

renom as blocking.

Comment 10

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

+    // The preferred address is already in use.  Did the OS give us a
+    // consolation prize?
+    if (result != RESERVE_FAILED) {
+      ARENA_POISON = candidate + rgnsize/2 - 1;
+      return PR_SUCCESS;
+    }

Shouldn't this be ARENA_POISON = result + rgnsize/2 - 1?

+    result = ReserveRegion(0, rgnsize);
+    if (result != RESERVE_FAILED) {
+      ARENA_POISON = candidate + rgnsize/2 - 1;
+      return PR_SUCCESS;
     }

Here too?

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 414160 [details] [diff] [review]
possible fix (for real)

I know nothing about the poison arena, but I do know that assuming certain anything about memory layout is a bad idea and this patch avoids that, so r=me with roc's nits fixed.

BTW, what's so special about 0xF0DEAFFF that the search starts there?

Comment 12

[Zack Weinberg (:zwol)]

(In reply to comment #10)
> +    // The preferred address is already in use.  Did the OS give us a
> +    // consolation prize?
> +    if (result != RESERVE_FAILED) {
> +      ARENA_POISON = candidate + rgnsize/2 - 1;
> +      return PR_SUCCESS;
> +    }
> 
> Shouldn't this be ARENA_POISON = result + rgnsize/2 - 1?

Yes, it should; thanks for catching that.  I really do need to refactor this stuff so it's shared with the test case and is consistent about types and stuff.  Revised minimal-change patch on Monday (I'll do the refactor in a separate bug).

(In reply to comment #11)
> (From update of attachment 414160 [details] [diff] [review])
> I know nothing about the poison arena, but I do know that assuming certain
> anything about memory layout is a bad idea and this patch avoids that, so r=me
> with roc's nits fixed.

Thanks.

> BTW, what's so special about 0xF0DEAFFF that the search starts there?

Just that that's what chofmann's scripts are looking for to distinguish poisoning crashes from others, so we want to get that address if possible.  The idea of the "four megs in either direction" scan was that it would at least preserve the 0xF0D part, but making the browser start up reliably seems more important at this point.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to comment #3)
> is there a way to inject application annotations into minidumps?

Bug 517133 comment 1 describes one possible way to do this.

Comment 14

[Zack Weinberg (:zwol)]

(In reply to comment #13)
> (In reply to comment #3)
> > is there a way to inject application annotations into minidumps?
> 
> Bug 517133 comment 1 describes one possible way to do this.

Thanks.  It looks like from Gecko-internal code I want to #include <nsExceptionHandler.h> and
then call CrashReporter::AppendAppNotesToCrashReport().  Can you confirm?

Comment 15

[Zack Weinberg (:zwol)]

per karl, Ted M is the person to ask about this.

Ted: We want to annotate crash reports with the address chosen for frame poisoning (which cannot, unfortunately, be fixed at compile time) so that they can be identified reliably in crash-stats.  The code that chooses the address is in libgklayout.

1) Can I #include <nsExceptionHandler.h> and use CrashReporter:: functions directly, or do I have to do something more XPCOMful?
2) Which is more appropriate: AnnotateCrashReport with a new key, AppendAppNotesToCrashReport, or something else?

Comment 16

[(not currently active) Ted Mielczarek]

(In reply to comment #15)
 
> 1) Can I #include <nsExceptionHandler.h> and use CrashReporter:: functions
> directly, or do I have to do something more XPCOMful?

Sadly, no, you'll need to use XPCOM, like this:
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsObjCExceptions.h#75

> 2) Which is more appropriate: AnnotateCrashReport with a new key,
> AppendAppNotesToCrashReport, or something else?

It depends on what you want to do with the data. I'm assuming that this is going to be attached to every report. The "App Notes" API is designed for attaching arbitrary text to a report that is intended to be human readable on the crash report page. If you want the data to be available for some sort of automated querying/reporting, then you'll want to use AnnotateCrashReport with a new key. The caveat there is that you'll then need to file a Socorro bug to get the server to store and display your new data.

Comment 17

[Zack Weinberg (:zwol)]

Attachment: patch as committed

Pushed http://hg.mozilla.org/mozilla-central/rev/16b7c8b2bcc9 with this version of the patch - the only change is s/candidate/PRUword(result)/ in two places as pointed out above by roc.

Comment 18

[Zack Weinberg (:zwol)]

I filed followup bugs for the crash annotations (bug 531845 and bug 531847) and for the refactory (bug 531848).

Comment 20

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/346d2bd20b25