Skip to content

Commit

Permalink
[Mac] Implement base::EnableTerminationOnHeapCorruption() by overridi…
Browse files Browse the repository at this point in the history
…ng 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.

Review URL: http://codereview.chromium.org/7670025

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@97315 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
rsesek@chromium.org committed Aug 18, 2011
1 parent 7f8b26b commit 1e1e5df
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 4 deletions.
1 change: 1 addition & 0 deletions base/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ include_rules = [
"+third_party/libevent",
"+third_party/dmg_fp",
"+third_party/GTM",
"+third_party/mach_override",
"+third_party/modp_b64",
"+third_party/tcmalloc",

Expand Down
3 changes: 3 additions & 0 deletions base/base.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,9 @@
'$(SDKROOT)/System/Library/Frameworks/Security.framework',
],
},
'dependencies': [
'../third_party/mach_override/mach_override.gyp:mach_override',
],
}],
[ 'OS != "win"', {
'dependencies': ['../third_party/libevent/libevent.gyp:libevent'],
Expand Down
4 changes: 4 additions & 0 deletions base/process_util_linux.cc
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,10 @@ int posix_memalign(void** ptr, size_t alignment, size_t size) {
#endif // !defined(USE_TCMALLOC)
} // extern C

void EnableTerminationOnHeapCorruption() {
// On Linux, there nothing to do AFAIK.
}

void EnableTerminationOnOutOfMemory() {
#if defined(OS_ANDROID)
// Android doesn't support setting a new handler.
Expand Down
85 changes: 85 additions & 0 deletions base/process_util_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include <mach/mach_vm.h>
#include <mach/shared_region.h>
#include <mach/task.h>
#include <mach-o/dyld.h>
#include <mach-o/nlist.h>
#include <malloc/malloc.h>
#import <objc/runtime.h>
#include <spawn.h>
Expand All @@ -34,6 +36,7 @@
#include "base/time.h"
#include "third_party/apple_apsl/CFBase.h"
#include "third_party/apple_apsl/malloc.h"
#include "third_party/mach_override/mach_override.h"

namespace base {

Expand Down Expand Up @@ -483,6 +486,88 @@ size_t GetSystemCommitCharge() {
return (data.active_count * page_size) / 1024;
}

namespace {

// Finds the library path for malloc() and thus the libC part of libSystem,
// which in Lion is in a separate image.
const char* LookUpLibCPath() {
const void* addr = reinterpret_cast<void*>(&malloc);

Dl_info info;
if (dladdr(addr, &info))
return info.dli_fname;

LOG(WARNING) << "Could not find image path for malloc()";
return NULL;
}

typedef void(*malloc_error_break_t)(void);
malloc_error_break_t g_original_malloc_error_break = NULL;

// Returns the function pointer for malloc_error_break. This symbol is declared
// as __private_extern__ and cannot be dlsym()ed. Instead, use nlist() to
// get it.
malloc_error_break_t LookUpMallocErrorBreak() {
#if ARCH_CPU_32_BITS
const char* lib_c_path = LookUpLibCPath();
if (!lib_c_path)
return NULL;

// Only need to look up two symbols, but nlist() requires a NULL-terminated
// array and takes no count.
struct nlist nl[3];
bzero(&nl, sizeof(nl));

// The symbol to find.
nl[0].n_un.n_name = const_cast<char*>("_malloc_error_break");

// A reference symbol by which the address of the desired symbol will be
// calculated.
nl[1].n_un.n_name = const_cast<char*>("_malloc");

int rv = nlist(lib_c_path, nl);
if (rv != 0 || nl[0].n_type == N_UNDF || nl[1].n_type == N_UNDF) {
return NULL;
}

// nlist() returns addresses as offsets in the image, not the instruction
// pointer in memory. Use the known in-memory address of malloc()
// to compute the offset for malloc_error_break().
uintptr_t reference_addr = reinterpret_cast<uintptr_t>(&malloc);
reference_addr -= nl[1].n_value;
reference_addr += nl[0].n_value;

return reinterpret_cast<malloc_error_break_t>(reference_addr);
#endif // ARCH_CPU_32_BITS

return NULL;
}

void CrMallocErrorBreak() {
g_original_malloc_error_break();
LOG(ERROR) <<
"Terminating process due to a potential for future heap corruption";
base::debug::BreakDebugger();
}

} // namespace

void EnableTerminationOnHeapCorruption() {
malloc_error_break_t malloc_error_break = LookUpMallocErrorBreak();
if (!malloc_error_break) {
LOG(WARNING) << "Could not find malloc_error_break";
return;
}

mach_error_t err = mach_override_ptr(
(void*)malloc_error_break,
(void*)&CrMallocErrorBreak,
(void**)&g_original_malloc_error_break);

if (err != err_none)
LOG(WARNING) << "Could not override malloc_error_break; error = " << err;
}

// ------------------------------------------------------------------------

namespace {
Expand Down
4 changes: 0 additions & 4 deletions base/process_util_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -656,10 +656,6 @@ bool LaunchProcess(const CommandLine& cmdline,

ProcessMetrics::~ProcessMetrics() { }

void EnableTerminationOnHeapCorruption() {
// On POSIX, there nothing to do AFAIK.
}

bool EnableInProcessStackDumping() {
// When running in an application, our code typically expects SIGPIPE
// to be ignored. Therefore, when testing that same code, it should run
Expand Down
14 changes: 14 additions & 0 deletions base/process_util_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,20 @@ TEST_F(ProcessUtilTest, LaunchAsUser) {

#endif // defined(OS_WIN)

#if defined(OS_MACOSX)

TEST_F(ProcessUtilTest, MacTerminateOnHeapCorruption) {
// Note that base::EnableTerminationOnHeapCorruption() is called as part of
// test suite setup and does not need to be done again, else mach_override
// will fail.

char buf[3];
ASSERT_DEATH(free(buf), "pointer being freed was not allocated.*"
"Terminating process due to a potential for future heap corruption");
}

#endif // defined(OS_MACOSX)

#if defined(OS_POSIX)

namespace {
Expand Down

0 comments on commit 1e1e5df

Please sign in to comment.