[Mac] Implement base::EnableTerminationOnHeapCorruption() by overriding malloc_error_break().

[Mac] Implement base::EnableTerminationOnHeapCorruption() by overriding malloc_error_break().

This makes malloc_error_break() fatal for all processes in an attempt to get
better stack traces when heap corruption may occur.

BUG=90884, 91068, 93191
TEST=See bug 93191 for repro steps. A crash report gets generated with a hopefully more-useful stack.

Originally landed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97315
Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=97322

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97351

[Mark Mentovai, 2011-08-17 16:44:54]

I’m glad you took this, because you did a good job with it and it freed me up to
actually track down the heap corruption (http://crbug.com/93191). This
absolutely would have led us to the source of the corruption much sooner.

Can you provide a test for this? Something along the lines of a death test that
does a double-free would probably be good. I don’t see a good way to do a death
test for use-after-free: there’s no guarantee that malloc will look at the
memory we scribbled on after free, because there’s no guarantee it’ll dole out
the same block repeatedly.

http://codereview.chromium.org/7670025/diff/1/base/process_util_linux.cc
File base/process_util_linux.cc (right):

http://codereview.chromium.org/7670025/diff/1/base/process_util_linux.cc#newc...
base/process_util_linux.cc:722: // On POSIX, there nothing to do AFAIK.
This comment should now say Linux, not POSIX.

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm
File base/process_util_mac.mm (right):

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:491: typedef void(*malloc_error_break_t)(void);
Move the typedef and declaration down so that they’re above CrMallocErrorBreak,
the first place they’re needed.

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:496: const char* LookupLibSystemPath() {
Since the “part of libSystem” is what Apple calls Libc (or
system/libsystem_c.dylib), this should probably refer to Libc or LibC,
LibSystemC, or LibSystemLibc or LibSystemLibC. (Libc has always been distinct in
source, but is only distinct in runtime binary as of Lion.)

“Look up” as a verb is two words, so InterCapped, this should be LookUpLibcPath
(or LookUpLibCPath). Same on line 510.

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:509: // get it.
I think I’ll have to (or someone else will have to) write an nlist equivalent
that handles the 64-bit world. When I (or someone else) does this, it should be
possible to do without the stupid interface that opens a new copy of the
library. We should be able to do a better job and write something with a better
interface.

I actually kind of want to do this to give myself an nlist equivalent I can use
to test for the presence of symbols with N_UNDF for the
closure_blocks_leopard_compat unit test.

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:510: malloc_error_break_t LookupMallocErrorBreak() {
LookUp (as above).

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:512: const char* lib_system_path =
LookupLibSystemPath();
Rename the variable if you rename the function.

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:516: // Only need to lookup two symbols, but nlist()
requires a NULL-terminated
Look up should be two words here too.

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:517: // array and takes no count. So do that.
Nix “So do that.”

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:522: nl[0].n_un.n_name =
const_cast<char*>("_malloc_error_break");
Wow, what a terrible interface.

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:529: if (rv < 0 || nl[0].n_type == N_UNDF ||
nl[1].n_type == N_UNDF) {
Shouldn’t you explicitly test |rv == 0| here?

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:554: malloc_error_break_t error_break =
LookupMallocErrorBreak();
I’d call this malloc_error_break.

http://codereview.chromium.org/7670025/diff/1/base/process_util_mac.mm#newcod...
base/process_util_mac.mm:566: LOG(WARNING) << "Could not override
malloc_error_break; error = " << err;
bpoop has a custom set of logging streams to handle mach_error_t. :)

(No change required, just sayin’.)

[Robert Sesek, 2011-08-17 19:32:01]

Addressed all comments and added a test. I wished all tests were that easy/fun
to write...

[Mark Mentovai, 2011-08-17 21:16:29]

LGTM

[Robert Sesek, 2011-08-18 13:43:29]

So the double-free part of the test does not work across all our machines/OSes.
The free()ing of stack-allocated data does, though. Would you be ok with just
that? If so, will land.

[Mark Mentovai, 2011-08-18 14:30:03]

rsesek@chromium.org wrote:
> So the double-free part of the test does not work across all our
> machines/OSes.

On which does it not work? Have you checked the Libc source to see if
it’s just something that we can deal with on our side? Do things
change if we do a small or large allocation instead of a tiny one?

> The free()ing of stack-allocated data does, though. Would you be ok with
> just that? If so, will land.

I would be OK with that.

If we can deterministically tell whether or not the double-free check
would work (like with an OS version check), I’d prefer to keep the
double-free check in situations where we’re positive it’d be good.

[Robert Sesek, 2011-08-18 14:37:24]

On 2011/08/18 14:30:03, Mark Mentovai wrote:
> mailto:rsesek@chromium.org wrote:
> > So the double-free part of the test does not work across all our
> > machines/OSes.
> 
> On which does it not work? Have you checked the Libc source to see if
> it’s just something that we can deal with on our side? Do things
> change if we do a small or large allocation instead of a tiny one?

I ran it on my 10.6 laptop and it worked, but it failed on the 10.6 trybots.

> > The free()ing of stack-allocated data does, though. Would you be ok with
> > just that? If so, will land.
> 
> I would be OK with that.
> 
> If we can deterministically tell whether or not the double-free check
> would work (like with an OS version check), I’d prefer to keep the
> double-free check in situations where we’re positive it’d be good.

I read the libc source and the issue is that it'll put the block onto the tiny
free list. We could probably craft more allocations and deallocations to try and
get the block recycled to the point where the double-free would trip correctly,
but it wouldn't be guaranteed deterministic, which seems bad for a test. I also
tried writing 0xdeadbeef into the buffer after the first free to break the
checksum on the second free, but that didn't work either.

[Robert Sesek, 2011-08-18 14:39:50]

On 2011/08/18 14:30:03, Mark Mentovai wrote:
>Do things change if we do a small or large allocation instead of a tiny one?

Forgot to address this part: sometimes. When I tried with a smaller size (3
bytes) it didn't trip at all on SnoLo or Lion. 1024 seems to trip on Lion, but
not certain SnoLos. I also tried doing non-base-two sizes, freeing, and then
reallocating a smaller base-two sized block to see if it'd carve up the
allocations differently, but that also did not work.

[Mark Mentovai, 2011-08-18 15:22:11]

rsesek@chromium.org wrote:
> On 2011/08/18 14:30:03, Mark Mentovai wrote:
>> If we can deterministically tell whether or not the double-free check
>> would work (like with an OS version check), I’d prefer to keep the
>> double-free check in situations where we’re positive it’d be good.
>
> I read the libc source and the issue is that it'll put the block onto the tiny
> free list. We could probably craft more allocations and deallocations to try
and
> get the block recycled to the point where the double-free would trip
correctly,
> but it wouldn't be guaranteed deterministic, which seems bad for a test.

Right. I’d rather have one deterministic test, than adding flaky tests to it.

> I also
> tried writing 0xdeadbeef into the buffer after the first free to break the
> checksum on the second free, but that didn't work either.

This wouldn’t work. The malloc checksum checks happen when you go to
malloc a new block, and malloc wants to give out memory that had
previously been freed. You’d need malloc-free-scribble-malloc, but
there’s no way to guarantee that the second malloc would try to
recycle the block used by the first.

[Mark Mentovai, 2011-08-18 15:37:51]

rsesek@chromium.org wrote:
> Forgot to address this part: sometimes. When I tried with a smaller size (3
> bytes) it didn't trip at all on SnoLo or Lion. 1024 seems to trip on Lion, but
> not certain SnoLos. I also tried doing non-base-two sizes, freeing, and then
> reallocating a smaller base-two sized block to see if it'd carve up the
> allocations differently, but that also did not work.

Just wondering if this would work. Feel free to check in without the
double-free death test and leave only the I-try-to-free-stack-memory
death test.

  void* blocks[5];
  for (int block_index = 0; block_index < arraysize(blocks); ++block_index) {
    blocks[block_index] = malloc(1024);
  }

  free(blocks[1]);
  free(blocks[3]);

  free(blocks[1]);
  free(blocks[3]);

[Robert Sesek, 2011-08-18 19:50:02]

Ok. With mach_override patch now.

[Mark Mentovai, 2011-08-18 20:21:07]

LGTM

[Scott Hess - ex-Googler, 2011-08-22 19:50:34]

http://codereview.chromium.org/7670025/diff/2005/base/process_util_mac.mm
File base/process_util_mac.mm (right):

http://codereview.chromium.org/7670025/diff/2005/base/process_util_mac.mm#new...
base/process_util_mac.mm:510: malloc_error_break_t LookUpMallocErrorBreak() {
Would it be worthwhile to unify this with the comparable code in
chrome/browser/ui/cocoa/objc_zombie.mm ?  Or would that just encourage this kind
of stuff?

[Robert Sesek, 2011-08-22 19:52:14]

http://codereview.chromium.org/7670025/diff/2005/base/process_util_mac.mm
File base/process_util_mac.mm (right):

http://codereview.chromium.org/7670025/diff/2005/base/process_util_mac.mm#new...
base/process_util_mac.mm:510: malloc_error_break_t LookUpMallocErrorBreak() {
On 2011/08/22 19:50:35, shess wrote:
> Would it be worthwhile to unify this with the comparable code in
> chrome/browser/ui/cocoa/objc_zombie.mm ?  Or would that just encourage this
kind
> of stuff?

We want to explicitly discourage this kind of very naughty behavior. Mark says
we should do some unification when we have to do 32- and 64-bit versions,
though.

[Mark Mentovai, 2011-08-22 20:07:23]

http://codereview.chromium.org/7670025/diff/2005/base/process_util_mac.mm
File base/process_util_mac.mm (right):

http://codereview.chromium.org/7670025/diff/2005/base/process_util_mac.mm#new...
base/process_util_mac.mm:510: malloc_error_break_t LookUpMallocErrorBreak() {
shess wrote:
> Would it be worthwhile to unify this with the comparable code in
> chrome/browser/ui/cocoa/objc_zombie.mm ?  Or would that just encourage this
kind
> of stuff?

My previous review comments on this thread tell you where I think we need to go
with this.