From 37a3b8d997aa22da0f7ec741d443f332f58b3eda Mon Sep 17 00:00:00 2001 From: "thestig@chromium.org" Date: Tue, 23 Sep 2014 20:30:09 +0000 Subject: [PATCH] Linux: Call memset() in a couple places in ExceptionHandler to avoid uninit memory reads under Valgrind. Also move private static variables into the .cc file. BUG=chromium:332335 R=ivanpe@chromium.org Review URL: https://breakpad.appspot.com/5734002 git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@1385 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/client/linux/handler/exception_handler.cc | 52 ++++++++++--------- src/client/linux/handler/exception_handler.h | 15 ++---- 2 files changed, 32 insertions(+), 35 deletions(-) diff --git a/src/client/linux/handler/exception_handler.cc b/src/client/linux/handler/exception_handler.cc index f30e4594..db2f1e62 100644 --- a/src/client/linux/handler/exception_handler.cc +++ b/src/client/linux/handler/exception_handler.cc @@ -68,6 +68,7 @@ #include #include #include +#include #include #include #include @@ -150,6 +151,7 @@ void InstallAlternateStackLocked() { old_stack.ss_size < kSigStackSize) { new_stack.ss_sp = malloc(kSigStackSize); new_stack.ss_size = kSigStackSize; + memset(new_stack.ss_sp, 0, kSigStackSize); if (sys_sigaltstack(&new_stack, NULL) == -1) { free(new_stack.ss_sp); @@ -186,13 +188,13 @@ void RestoreAlternateStackLocked() { stack_installed = false; } -} // namespace +// The global exception handler stack. This is need because there may exist +// multiple ExceptionHandler instances in a process. Each will have itself +// registered in this stack. +std::vector* g_handler_stack_ = NULL; +pthread_mutex_t g_handler_stack_mutex_ = PTHREAD_MUTEX_INITIALIZER; -// We can stack multiple exception handlers. In that case, this is the global -// which holds the stack. -std::vector* ExceptionHandler::handler_stack_ = NULL; -pthread_mutex_t ExceptionHandler::handler_stack_mutex_ = - PTHREAD_MUTEX_INITIALIZER; +} // namespace // Runs before crashing: normal context. ExceptionHandler::ExceptionHandler(const MinidumpDescriptor& descriptor, @@ -212,30 +214,30 @@ ExceptionHandler::ExceptionHandler(const MinidumpDescriptor& descriptor, if (!IsOutOfProcess() && !minidump_descriptor_.IsFD()) minidump_descriptor_.UpdatePath(); - pthread_mutex_lock(&handler_stack_mutex_); - if (!handler_stack_) - handler_stack_ = new std::vector; + pthread_mutex_lock(&g_handler_stack_mutex_); + if (!g_handler_stack_) + g_handler_stack_ = new std::vector; if (install_handler) { InstallAlternateStackLocked(); InstallHandlersLocked(); } - handler_stack_->push_back(this); - pthread_mutex_unlock(&handler_stack_mutex_); + g_handler_stack_->push_back(this); + pthread_mutex_unlock(&g_handler_stack_mutex_); } // Runs before crashing: normal context. ExceptionHandler::~ExceptionHandler() { - pthread_mutex_lock(&handler_stack_mutex_); + pthread_mutex_lock(&g_handler_stack_mutex_); std::vector::iterator handler = - std::find(handler_stack_->begin(), handler_stack_->end(), this); - handler_stack_->erase(handler); - if (handler_stack_->empty()) { - delete handler_stack_; - handler_stack_ = NULL; + std::find(g_handler_stack_->begin(), g_handler_stack_->end(), this); + g_handler_stack_->erase(handler); + if (g_handler_stack_->empty()) { + delete g_handler_stack_; + g_handler_stack_ = NULL; RestoreAlternateStackLocked(); RestoreHandlersLocked(); } - pthread_mutex_unlock(&handler_stack_mutex_); + pthread_mutex_unlock(&g_handler_stack_mutex_); } // Runs before crashing: normal context. @@ -295,7 +297,7 @@ void ExceptionHandler::RestoreHandlersLocked() { // static void ExceptionHandler::SignalHandler(int sig, siginfo_t* info, void* uc) { // All the exception signals are blocked at this point. - pthread_mutex_lock(&handler_stack_mutex_); + pthread_mutex_lock(&g_handler_stack_mutex_); // Sometimes, Breakpad runs inside a process where some other buggy code // saves and restores signal handlers temporarily with 'signal' @@ -322,13 +324,13 @@ void ExceptionHandler::SignalHandler(int sig, siginfo_t* info, void* uc) { // default one to avoid an infinite loop here. signal(sig, SIG_DFL); } - pthread_mutex_unlock(&handler_stack_mutex_); + pthread_mutex_unlock(&g_handler_stack_mutex_); return; } bool handled = false; - for (int i = handler_stack_->size() - 1; !handled && i >= 0; --i) { - handled = (*handler_stack_)[i]->HandleSignal(sig, info, uc); + for (int i = g_handler_stack_->size() - 1; !handled && i >= 0; --i) { + handled = (*g_handler_stack_)[i]->HandleSignal(sig, info, uc); } // Upon returning from this signal handler, sig will become unmasked and then @@ -342,7 +344,7 @@ void ExceptionHandler::SignalHandler(int sig, siginfo_t* info, void* uc) { RestoreHandlersLocked(); } - pthread_mutex_unlock(&handler_stack_mutex_); + pthread_mutex_unlock(&g_handler_stack_mutex_); if (info->si_pid || sig == SIGABRT) { // This signal was triggered by somebody sending us the signal with kill(). @@ -398,6 +400,8 @@ bool ExceptionHandler::HandleSignal(int sig, siginfo_t* info, void* uc) { sys_prctl(PR_SET_DUMPABLE, 1, 0, 0, 0); } CrashContext context; + // Fill in all the holes in the struct to make Valgrind happy. + memset(&context, 0, sizeof(context)); memcpy(&context.siginfo, info, sizeof(siginfo_t)); memcpy(&context.context, uc, sizeof(struct ucontext)); #if defined(__aarch64__) @@ -449,7 +453,7 @@ bool ExceptionHandler::GenerateDump(CrashContext *context) { // of caution than smash it into random locations. static const unsigned kChildStackSize = 16000; PageAllocator allocator; - uint8_t* stack = (uint8_t*) allocator.Alloc(kChildStackSize); + uint8_t* stack = reinterpret_cast(allocator.Alloc(kChildStackSize)); if (!stack) return false; // clone() needs the top-most address. (scrub just to be safe) diff --git a/src/client/linux/handler/exception_handler.h b/src/client/linux/handler/exception_handler.h index bb88b950..591c3108 100644 --- a/src/client/linux/handler/exception_handler.h +++ b/src/client/linux/handler/exception_handler.h @@ -30,15 +30,13 @@ #ifndef CLIENT_LINUX_HANDLER_EXCEPTION_HANDLER_H_ #define CLIENT_LINUX_HANDLER_EXCEPTION_HANDLER_H_ -#include -#include - -#include #include #include #include #include +#include + #include "client/linux/crash_generation/crash_generation_client.h" #include "client/linux/handler/minidump_descriptor.h" #include "client/linux/minidump_writer/minidump_writer.h" @@ -129,7 +127,7 @@ class ExceptionHandler { ExceptionHandler(const MinidumpDescriptor& descriptor, FilterCallback filter, MinidumpCallback callback, - void *callback_context, + void* callback_context, bool install_handler, const int server_fd); ~ExceptionHandler(); @@ -228,6 +226,7 @@ class ExceptionHandler { // Report a crash signal from an SA_SIGINFO signal handler. bool HandleSignal(int sig, siginfo_t* info, void* uc); + private: // Save the old signal handlers and install new ones. static bool InstallHandlersLocked(); @@ -258,12 +257,6 @@ class ExceptionHandler { // believes are never read. volatile HandlerCallback crash_handler_; - // The global exception handler stack. This is need becuase there may exist - // multiple ExceptionHandler instances in a process. Each will have itself - // registered in this stack. - static std::vector *handler_stack_; - static pthread_mutex_t handler_stack_mutex_; - // We need to explicitly enable ptrace of parent processes on some // kernels, but we need to know the PID of the cloned process before we // can do this. We create a pipe which we can use to block the