From 1e4be91918b614e74a3882132fa4442e758f0e4d Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 24 Jan 2017 13:31:20 -0500 Subject: [PATCH 01/11] =?UTF-8?q?mac:=20Faster=20bit=20testing=20for=20EXC?= =?UTF-8?q?=5FGUARD=20exception=20=E2=80=9Cflavors=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After e7630628e9c9, I thought “isn’t there a standard library function for that?” There is! Change-Id: I284c7fdf8535c4fc53100e80fceb363bf2afee93 Reviewed-on: https://chromium-review.googlesource.com/431856 Reviewed-by: Scott Graham --- util/mach/exception_types.cc | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/util/mach/exception_types.cc b/util/mach/exception_types.cc index 85bdf770..d33fa937 100644 --- a/util/mach/exception_types.cc +++ b/util/mach/exception_types.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include "base/logging.h" #include "base/mac/mach_logging.h" @@ -200,16 +201,19 @@ int32_t ExceptionCodeForMetrics(exception_type_t exception, // one another. For the purposes of encoding these codes for metrics, // convert the flavor codes to their corresponding bit shift values. const uint32_t guard_flavor = (code_0 >> 32) & 0x1fffffff; - uint8_t metrics_guard_flavor = 0xff; - for (int bit = 0; bit < 29; ++bit) { - const uint32_t test_mask = 1 << bit; - if (guard_flavor & test_mask) { - // Make sure that no other bits are set. - DCHECK_EQ(guard_flavor, test_mask); + const int first_bit = ffs(guard_flavor); + uint8_t metrics_guard_flavor; + if (first_bit) { + metrics_guard_flavor = first_bit - 1; - metrics_guard_flavor = bit; - break; + const uint32_t test_guard_flavor = 1 << metrics_guard_flavor; + if (guard_flavor != test_guard_flavor) { + // Another bit is set. + DCHECK_EQ(guard_flavor, test_guard_flavor); + metrics_guard_flavor = 0xff; } + } else { + metrics_guard_flavor = 0xff; } metrics_code_0 = (guard_type << 8) | metrics_guard_flavor; From 3e5ae2dc872a90e6fe2bc5870e896e5057d6ca24 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 27 Jan 2017 18:17:05 -0500 Subject: [PATCH 02/11] Update comments in IsExceptionNonfatalResource() given 10.12 source With reference to 10.12 source, commentary regarding RESOURCE_TYPE_IO can be authoritative. Cursory examination of 10.12 source reveals that RESOURCE_TYPE_MEMORY can now be fatal, although deeper examination reveals that this is impossible on macOS. State this authoritatively as well. BUG=crashpad:124 Change-Id: I52124c68fe017015983ab46e54006ba97ecd0142 Reviewed-on: https://chromium-review.googlesource.com/434297 Reviewed-by: Robert Sesek --- util/mach/exception_types.cc | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/util/mach/exception_types.cc b/util/mach/exception_types.cc index d33fa937..e133cb9a 100644 --- a/util/mach/exception_types.cc +++ b/util/mach/exception_types.cc @@ -309,15 +309,32 @@ bool IsExceptionNonfatalResource(exception_type_t exception, if (resource_type == RESOURCE_TYPE_MEMORY && resource_flavor == FLAVOR_HIGH_WATERMARK) { - // These exceptions are never fatal. See 10.10 + // These exceptions were never fatal prior to 10.12. See 10.10 // xnu-2782.1.97/osfmk/kern/task.c // THIS_PROCESS_CROSSED_HIGH_WATERMARK__SENDING_EXC_RESOURCE(). + // + // A superficial examination of 10.12 shows that these exceptions may be + // fatal, as determined by the P_MEMSTAT_FATAL_MEMLIMIT bit of the + // kernel-internal struct proc::p_memstat_state. See 10.12.3 + // xnu-3789.41.3/osfmk/kern/task.c task_footprint_exceeded(). This bit is + // not exposed to user space, which makes it difficult to determine whether + // the kernel considers a given instance of this exception fatal. However, a + // close read reveals that it is only possible for this bit to become set + // when xnu-3789.41.3/bsd/kern/kern_memorystatus.c + // memorystatus_cmd_set_memlimit_properties() is called, which is only + // possible when the kernel is built with CONFIG_JETSAM set, or if the + // kern.memorystatus_highwater_enabled sysctl is used, which is only + // possible when the kernel is built with DEVELOPMENT or DEBUG set. Although + // CONFIG_JETSAM is used on iOS, it is not used on macOS. DEVELOPMENT and + // DEBUG are also not set for production kernels. It therefore remains + // impossible for these exceptions to be fatal, even on 10.12. return true; } if (resource_type == RESOURCE_TYPE_IO) { - // These exceptions don’t ever appear to be fatal. See - // https://crashpad.chromium.org/bug/124. + // These exceptions are never fatal. See 10.12.3 + // xnu-3789.41.3/osfmk/kern/task.c + // SENDING_NOTIFICATION__THIS_PROCESS_IS_CAUSING_TOO_MUCH_IO(). return true; } From 56020daea9706c073d83c418e615a27559a3f379 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 30 Jan 2017 10:34:51 -0500 Subject: [PATCH 03/11] =?UTF-8?q?ExceptionTypes=20test:=20test=20=E2=80=9C?= =?UTF-8?q?naked=E2=80=9D=20signals?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since it’s possible to receive an EXC_CRASH for any signal that generates a core by default even if the signal did not originate from a Mach exception, update the tests to ensure that all such signals can be unwrapped from an exception properly. This happens when a signal such as SIGSEGV is sent with kill(), for example. Change-Id: I1ee32cc6943f21ae349fa6788430d074acff9ed8 Reviewed-on: https://chromium-review.googlesource.com/434717 Reviewed-by: Robert Sesek --- util/mach/exception_types_test.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/util/mach/exception_types_test.cc b/util/mach/exception_types_test.cc index 7ecdd252..4175c637 100644 --- a/util/mach/exception_types_test.cc +++ b/util/mach/exception_types_test.cc @@ -57,7 +57,13 @@ TEST(ExceptionTypes, ExcCrashRecoverOriginalException) { {0x0700080, EXC_SYSCALL, 128, 0}, {0x0706000, EXC_SYSCALL, 0x6000, 0}, {0x3000000, 0, 0, SIGQUIT}, + {0x4000000, 0, 0, SIGILL}, + {0x5000000, 0, 0, SIGTRAP}, {0x6000000, 0, 0, SIGABRT}, + {0x7000000, 0, 0, SIGEMT}, + {0x8000000, 0, 0, SIGFPE}, + {0xa000000, 0, 0, SIGBUS}, + {0xb000000, 0, 0, SIGSEGV}, {0xc000000, 0, 0, SIGSYS}, {0, 0, 0, 0}, }; @@ -181,7 +187,13 @@ TEST(ExceptionTypes, ExceptionCodeForMetrics) { #define ENCODE_EXC_CRASH_SIGNAL(signal) \ { EXC_CRASH, (((signal) & 0xff) << 24), (EXC_CRASH << 16) | (signal) } ENCODE_EXC_CRASH_SIGNAL(SIGQUIT), + ENCODE_EXC_CRASH_SIGNAL(SIGILL), + ENCODE_EXC_CRASH_SIGNAL(SIGTRAP), ENCODE_EXC_CRASH_SIGNAL(SIGABRT), + ENCODE_EXC_CRASH_SIGNAL(SIGEMT), + ENCODE_EXC_CRASH_SIGNAL(SIGFPE), + ENCODE_EXC_CRASH_SIGNAL(SIGBUS), + ENCODE_EXC_CRASH_SIGNAL(SIGSEGV), ENCODE_EXC_CRASH_SIGNAL(SIGSYS), #undef ENCODE_EXC_CRASH_SIGNAL From 7050c55fca1b4327e447b83b7657f6fa5e89697a Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Tue, 31 Jan 2017 14:12:19 -0800 Subject: [PATCH 04/11] Remove LazyInstance usage R=mark@chromium.org BUG=chromium:686866 Change-Id: I067988694f15d93b064d0b10b1bc5b908c9e5f52 Reviewed-on: https://chromium-review.googlesource.com/435441 Reviewed-by: Mark Mentovai --- util/thread/thread_log_messages.cc | 47 +++++++++++++----------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/util/thread/thread_log_messages.cc b/util/thread/thread_log_messages.cc index dec111e7..60be08b5 100644 --- a/util/thread/thread_log_messages.cc +++ b/util/thread/thread_log_messages.cc @@ -16,7 +16,6 @@ #include -#include "base/lazy_instance.h" #include "base/logging.h" #include "base/threading/thread_local_storage.h" @@ -28,14 +27,24 @@ namespace { // handler. A thread may register its thread-specific log message list to // receive messages produced just on that thread. // -// Only one object of this class may exist in the program at a time. There must -// not be any log message handler in effect when it is created, and nothing else -// can be set as a log message handler while an object of this class exists. -// -// Practically, the only object of this class that might exist is managed by the -// g_master lazy instance, which will create it upon first use. +// Only one object of this class may exist in the program at a time as created +// by GetInstance(). There must not be any log message handler in effect when it +// is created, and nothing else can be set as a log message handler while an +// object of this class exists. class ThreadLogMessagesMaster { public: + void SetThreadMessageList(std::vector* message_list) { + DCHECK_EQ(logging::GetLogMessageHandler(), &LogMessageHandler); + DCHECK_NE(tls_.Get() != nullptr, message_list != nullptr); + tls_.Set(message_list); + } + + static ThreadLogMessagesMaster* GetInstance() { + static auto master = new ThreadLogMessagesMaster(); + return master; + } + + private: ThreadLogMessagesMaster() { DCHECK(!tls_.initialized()); tls_.Initialize(nullptr); @@ -45,20 +54,8 @@ class ThreadLogMessagesMaster { logging::SetLogMessageHandler(LogMessageHandler); } - ~ThreadLogMessagesMaster() { - DCHECK_EQ(logging::GetLogMessageHandler(), &LogMessageHandler); - logging::SetLogMessageHandler(nullptr); + ~ThreadLogMessagesMaster() = delete; - tls_.Free(); - } - - void SetThreadMessageList(std::vector* message_list) { - DCHECK_EQ(logging::GetLogMessageHandler(), &LogMessageHandler); - DCHECK_NE(tls_.Get() != nullptr, message_list != nullptr); - tls_.Set(message_list); - } - - private: static bool LogMessageHandler(logging::LogSeverity severity, const char* file_path, int line, @@ -84,18 +81,14 @@ class ThreadLogMessagesMaster { base::ThreadLocalStorage::StaticSlot ThreadLogMessagesMaster::tls_ = TLS_INITIALIZER; -base::LazyInstance::Leaky g_master = - LAZY_INSTANCE_INITIALIZER; - } // namespace -ThreadLogMessages::ThreadLogMessages() - : log_messages_() { - g_master.Get().SetThreadMessageList(&log_messages_); +ThreadLogMessages::ThreadLogMessages() : log_messages_() { + ThreadLogMessagesMaster::GetInstance()->SetThreadMessageList(&log_messages_); } ThreadLogMessages::~ThreadLogMessages() { - g_master.Get().SetThreadMessageList(nullptr); + ThreadLogMessagesMaster::GetInstance()->SetThreadMessageList(nullptr); } } // namespace crashpad From 948fd2d019cfeef7658ada6c021de422bb31325c Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 7 Feb 2017 13:55:48 -0500 Subject: [PATCH 05/11] mac: Report a metric for handler crashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This installs signal handlers in the crashpad_handler process to log these crashes via the Crashpad.HandlerCrash.ExceptionCode.Mac histogram. This is roughly the same mechanism that’s used for Windows. The signal handler tries fairly hard to avoid swallowing signals, so that things appear to outside observers (including debuggers and crash handlers) identically to how they would look if no signal handler was present. The signal handler uses a different mapping schema than the existing Crashpad.ExceptionCode.Mac histogram for reasons explained in code comments. Because the mappings should not overlap, the new values may be added to the existing CrashpadMacExceptionCodes enum. BUG=crashpad:100 Change-Id: I9b8bda1c59d0a180501c285cdc672840a54f5efc Reviewed-on: https://chromium-review.googlesource.com/435451 Reviewed-by: Robert Sesek --- handler/handler_main.cc | 136 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 129 insertions(+), 7 deletions(-) diff --git a/handler/handler_main.cc b/handler/handler_main.cc index 3ada8c3e..9433a6af 100644 --- a/handler/handler_main.cc +++ b/handler/handler_main.cc @@ -14,9 +14,11 @@ #include "handler/handler_main.h" +#include #include #include #include +#include #include #include @@ -38,6 +40,8 @@ #include "handler/prune_crash_reports_thread.h" #include "tools/tool_support.h" #include "util/file/file_io.h" +#include "util/misc/metrics.h" +#include "util/numeric/in_range_cast.h" #include "util/stdlib/map_insert.h" #include "util/stdlib/string_number_conversion.h" #include "util/string/split_string.h" @@ -105,6 +109,122 @@ void Usage(const base::FilePath& me) { #if defined(OS_MACOSX) +struct sigaction g_original_crash_sigaction[NSIG]; + +void HandleCrashSignal(int sig, siginfo_t* siginfo, void* context) { + // Is siginfo->si_code useful? The only interesting values on macOS are 0 (not + // useful, signals generated asynchronously such as by kill() or raise()) and + // small positive numbers (useful, signal generated via a hardware fault). The + // standard specifies these other constants, and while xnu never uses them, + // they are intended to denote signals generated asynchronously and are + // included here. Additionally, existing practice on other systems + // (acknowledged by the standard) is for negative numbers to indicate that a + // signal was generated asynchronously. Although xnu does not do this, allow + // for the possibility for completeness. + bool si_code_valid = !(siginfo->si_code <= 0 || + siginfo->si_code == SI_USER || + siginfo->si_code == SI_QUEUE || + siginfo->si_code == SI_TIMER || + siginfo->si_code == SI_ASYNCIO || + siginfo->si_code == SI_MESGQ); + + // 0x5343 = 'SC', signifying “signal and code”, disambiguates from the schema + // used by ExceptionCodeForMetrics(). That system primarily uses Mach + // exception types and codes, which are not available to a POSIX signal + // handler. It does provide a way to encode only signal numbers, but does so + // with the understanding that certain “raw” signals would not be encountered + // without a Mach exception. Furthermore, it does not allow siginfo->si_code + // to be encoded, because that’s not available to Mach exception handlers. It + // would be a shame to lose that information available to a POSIX signal + // handler. + int metrics_code = 0x53430000 | (InRangeCast(sig, 0xff) << 8); + if (si_code_valid) { + metrics_code |= InRangeCast(siginfo->si_code, 0xff); + } + Metrics::HandlerCrashed(metrics_code); + + // Restore the previous signal handler. + DCHECK_GT(sig, 0); + DCHECK_LT(static_cast(sig), arraysize(g_original_crash_sigaction)); + struct sigaction* osa = &g_original_crash_sigaction[sig]; + int rv = sigaction(sig, osa, nullptr); + DPLOG_IF(ERROR, rv != 0) << "sigaction " << sig; + + // If the signal was received synchronously resulting from a hardware fault, + // returning from the signal handler will cause the kernel to re-raise it, + // because this handler hasn’t done anything to alleviate the condition that + // caused the signal to be raised in the first place. With the old signal + // handler in place (expected to be SIG_DFL), it will cause the same behavior + // to be taken as though this signal handler had never been installed at all + // (expected to be a crash). This is ideal, because the signal is re-raised + // with the same properties and from the same context that initially triggered + // it, providing the best debugging experience. + + if ((sig != SIGILL && sig != SIGFPE && sig != SIGBUS && sig != SIGSEGV) || + !si_code_valid) { + // Signals received other than via hardware faults, such as those raised + // asynchronously via kill() and raise(), and those arising via hardware + // traps such as int3 (resulting in SIGTRAP but advancing the instruction + // pointer), will not reoccur on their own when returning from the signal + // handler. Re-raise them or call to the previous signal handler as + // appropriate. + // + // Unfortunately, when SIGBUS is received asynchronously via kill(), + // siginfo->si_code makes it appear as though it was actually received via a + // hardware fault. See 10.12.3 xnu-3789.41.3/bsd/dev/i386/unix_signal.c + // sendsig(). An asynchronous SIGBUS will thus cause the handler-crashed + // metric to be logged but will not cause the process to terminate. This + // isn’t ideal, but asynchronous SIGBUS is an unexpected condition. The + // alternative, to re-raise here on any SIGBUS, is a bad idea because it + // would lose properties associated with the the original signal, which are + // very valuable for debugging and are visible to a Mach exception handler. + // Since SIGBUS is normally received synchronously in response to a hardware + // fault, don’t sweat the unexpected asynchronous case. + if (osa->sa_handler == SIG_DFL) { + // Because this signal handler executes with the signal blocked, this + // raise() cannot immediately deliver the signal. Delivery is deferred + // until this signal handler returns and the signal becomes unblocked. The + // re-raised signal will appear with the same context as where it was + // initially triggered. + rv = raise(sig); + DPLOG_IF(ERROR, rv != 0) << "raise"; + } else if (osa->sa_handler != SIG_IGN) { + if (osa->sa_flags & SA_SIGINFO) { + osa->sa_sigaction(sig, siginfo, context); + } else { + osa->sa_handler(sig); + } + } + } +} + +void InstallCrashHandler() { + struct sigaction sa = {}; + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_SIGINFO; + sa.sa_sigaction = HandleCrashSignal; + + // These are the core-generating signals from 10.12.3 + // xnu-3789.41.3/bsd/sys/signalvar.h sigprop: entries with SA_CORE are in the + // set. + const int kSignals[] = {SIGQUIT, + SIGILL, + SIGTRAP, + SIGABRT, + SIGEMT, + SIGFPE, + SIGBUS, + SIGSEGV, + SIGSYS}; + + for (int sig : kSignals) { + DCHECK_GT(sig, 0); + DCHECK_LT(static_cast(sig), arraysize(g_original_crash_sigaction)); + int rv = sigaction(sig, &sa, &g_original_crash_sigaction[sig]); + PCHECK(rv == 0) << "sigaction " << sig; + } +} + struct ResetSIGTERMTraits { static struct sigaction* InvalidValue() { return nullptr; @@ -126,9 +246,8 @@ void HandleSIGTERM(int sig, siginfo_t* siginfo, void* context) { g_exception_handler_server->Stop(); } -#endif // OS_MACOSX +#elif defined(OS_WIN) -#if defined(OS_WIN) LONG(WINAPI* g_original_exception_filter)(EXCEPTION_POINTERS*) = nullptr; LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { @@ -139,15 +258,18 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { else return EXCEPTION_CONTINUE_SEARCH; } -#endif // OS_WIN + +void InstallCrashHandler() { + g_original_exception_filter = + SetUnhandledExceptionFilter(&UnhandledExceptionHandler); +} + +#endif // OS_MACOSX } // namespace int HandlerMain(int argc, char* argv[]) { -#if defined(OS_WIN) - g_original_exception_filter = - SetUnhandledExceptionFilter(&UnhandledExceptionHandler); -#endif + InstallCrashHandler(); const base::FilePath argv0( ToolSupport::CommandLineArgumentToFilePathStringType(argv[0])); From 594eb43b589dd07b2461987ee2704171056ee768 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 7 Feb 2017 13:57:09 -0500 Subject: [PATCH 06/11] mac: Make 64-bit handler able to read 32-bit module lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 32-bit process_types definition of dyld_all_image_infos winds up with four extra bytes of tail padding when built into a 64-bit crashpad_handler compared to a 32-bit one, and compared to the structure’s native size. This prevents a 64-bit crashpad_handler from being able to create a module snapshot of a 32-bit process for dyld_all_image_infos versions 14 (since 10.9) and 15 (since 10.12). Work around this by placing a zero-length “end” marker and using offsetof(dyld_all_image_infos, end) in preference to sizeof(dyld_all_image_infos). BUG=crashpad:156,crashpad:148,crashpad:120 TEST=crashpad_snapshot_test ProcessTypes.DyldImagesSelf, run_with_crashpad --handler=crashpad_handler{,32} builtin_trap{,32} Change-Id: I406ad53851b5bd29ec802b7ad3073836ebe8c34c Reviewed-on: https://chromium-review.googlesource.com/437924 Reviewed-by: Robert Sesek --- snapshot/mac/process_types.h | 16 ++++- snapshot/mac/process_types/custom.cc | 65 ++++++++----------- .../mac/process_types/dyld_images.proctype | 7 ++ snapshot/mac/process_types/traits.h | 4 +- snapshot/mac/process_types_test.cc | 37 +++++++++++ 5 files changed, 86 insertions(+), 43 deletions(-) diff --git a/snapshot/mac/process_types.h b/snapshot/mac/process_types.h index 664b37b6..7274515f 100644 --- a/snapshot/mac/process_types.h +++ b/snapshot/mac/process_types.h @@ -27,13 +27,21 @@ namespace crashpad { namespace process_types { namespace internal { +// Some structure definitions have tail padding when built in a 64-bit +// environment, but will have less (or no) tail padding in a 32-bit environment. +// These structure’s apparent sizes will differ between these two environments, +// which is incorrect. Use this “Nothing” type to place an “end” marker after +// all of the real members of such structures, and use offsetof(type, end) to +// compute their actual sizes. +using Nothing = char[0]; + // Some structure definitions differ in 32-bit and 64-bit environments by having // additional “reserved” padding fields present only in the 64-bit environment. // These Reserved*_64Only* types allow the process_types system to replicate // these structures more precisely. -using Reserved32_64Only32 = char[0]; +using Reserved32_64Only32 = Nothing; using Reserved32_64Only64 = uint32_t; -using Reserved64_64Only32 = char[0]; +using Reserved64_64Only32 = Nothing; using Reserved64_64Only64 = uint64_t; } // namespace internal @@ -65,6 +73,7 @@ DECLARE_PROCESS_TYPE_TRAITS_CLASS(Generic, 64) using UIntPtr = internal::TraitsGeneric::UIntPtr; \ using Reserved32_64Only = internal::TraitsGeneric::Reserved32_64Only; \ using Reserved64_64Only = internal::TraitsGeneric::Reserved64_64Only; \ + using Nothing = internal::TraitsGeneric::Nothing; \ \ /* Initializes an object with data read from |process_reader| at \ * |address|, properly genericized. */ \ @@ -88,7 +97,7 @@ DECLARE_PROCESS_TYPE_TRAITS_CLASS(Generic, 64) /* Similar to Size(), but computes the expected size of a structure based \ * on the process’ bitness. This can be used prior to reading any data \ * from a process. */ \ - static size_t ExpectedSize(ProcessReader* process_reader); \ + static size_t ExpectedSize(ProcessReader* process_reader); #define PROCESS_TYPE_STRUCT_MEMBER(member_type, member_name, ...) \ member_type member_name __VA_ARGS__; @@ -155,6 +164,7 @@ DECLARE_PROCESS_TYPE_TRAITS_CLASS(Generic, 64) using UIntPtr = typename Traits::UIntPtr; \ using Reserved32_64Only = typename Traits::Reserved32_64Only; \ using Reserved64_64Only = typename Traits::Reserved64_64Only; \ + using Nothing = typename Traits::Nothing; \ \ /* Read(), ReadArrayInto(), and Size() are as in the generic user-visible \ * struct above. */ \ diff --git a/snapshot/mac/process_types/custom.cc b/snapshot/mac/process_types/custom.cc index 5fe92288..33e3a7ec 100644 --- a/snapshot/mac/process_types/custom.cc +++ b/snapshot/mac/process_types/custom.cc @@ -17,6 +17,8 @@ #include #include +#include + #include "base/logging.h" #include "snapshot/mac/process_types/internal.h" #include "util/mach/task_memory.h" @@ -62,46 +64,31 @@ bool ReadIntoVersioned(ProcessReader* process_reader, template size_t dyld_all_image_infos::ExpectedSizeForVersion( decltype(dyld_all_image_infos::version) version) { - if (version >= 14) { - return sizeof(dyld_all_image_infos); + const size_t kSizeForVersion[] = { + offsetof(dyld_all_image_infos, infoArrayCount), // 0 + offsetof(dyld_all_image_infos, libSystemInitialized), // 1 + offsetof(dyld_all_image_infos, jitInfo), // 2 + offsetof(dyld_all_image_infos, dyldVersion), // 3 + offsetof(dyld_all_image_infos, dyldVersion), // 4 + offsetof(dyld_all_image_infos, coreSymbolicationShmPage), // 5 + offsetof(dyld_all_image_infos, systemOrderFlag), // 6 + offsetof(dyld_all_image_infos, uuidArrayCount), // 7 + offsetof(dyld_all_image_infos, dyldAllImageInfosAddress), // 8 + offsetof(dyld_all_image_infos, initialImageCount), // 9 + offsetof(dyld_all_image_infos, errorKind), // 10 + offsetof(dyld_all_image_infos, sharedCacheSlide), // 11 + offsetof(dyld_all_image_infos, sharedCacheUUID), // 12 + offsetof(dyld_all_image_infos, infoArrayChangeTimestamp), // 13 + offsetof(dyld_all_image_infos, end), // 14 + }; + + if (version >= arraysize(kSizeForVersion)) { + return kSizeForVersion[arraysize(kSizeForVersion) - 1]; } - if (version >= 13) { - return offsetof(dyld_all_image_infos, infoArrayChangeTimestamp); - } - if (version >= 12) { - return offsetof(dyld_all_image_infos, sharedCacheUUID); - } - if (version >= 11) { - return offsetof(dyld_all_image_infos, sharedCacheSlide); - } - if (version >= 10) { - return offsetof(dyld_all_image_infos, errorKind); - } - if (version >= 9) { - return offsetof(dyld_all_image_infos, initialImageCount); - } - if (version >= 8) { - return offsetof(dyld_all_image_infos, dyldAllImageInfosAddress); - } - if (version >= 7) { - return offsetof(dyld_all_image_infos, uuidArrayCount); - } - if (version >= 6) { - return offsetof(dyld_all_image_infos, systemOrderFlag); - } - if (version >= 5) { - return offsetof(dyld_all_image_infos, coreSymbolicationShmPage); - } - if (version >= 3) { - return offsetof(dyld_all_image_infos, dyldVersion); - } - if (version >= 2) { - return offsetof(dyld_all_image_infos, jitInfo); - } - if (version >= 1) { - return offsetof(dyld_all_image_infos, libSystemInitialized); - } - return offsetof(dyld_all_image_infos, infoArrayCount); + + static_assert(std::is_unsigned::value, + "version must be unsigned"); + return kSizeForVersion[version]; } // static diff --git a/snapshot/mac/process_types/dyld_images.proctype b/snapshot/mac/process_types/dyld_images.proctype index eb8410aa..3470ee83 100644 --- a/snapshot/mac/process_types/dyld_images.proctype +++ b/snapshot/mac/process_types/dyld_images.proctype @@ -123,6 +123,13 @@ PROCESS_TYPE_STRUCT_BEGIN(dyld_all_image_infos) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_6) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_7) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_8) + + // The 32-bit version of the structure will have four extra bytes of tail + // padding when built for 64-bit systems than it does natively and when built + // for 32-bit systems. Instead of using sizeof(dyld_all_image_infos), use + // offsetof(dyld_all_image_infos, end) to avoid taking this tail padding into + // account. + PROCESS_TYPE_STRUCT_MEMBER(Nothing, end) PROCESS_TYPE_STRUCT_END(dyld_all_image_infos) #endif // ! PROCESS_TYPE_STRUCT_IMPLEMENT_INTERNAL_READ_INTO && diff --git a/snapshot/mac/process_types/traits.h b/snapshot/mac/process_types/traits.h index fec234d0..396799be 100644 --- a/snapshot/mac/process_types/traits.h +++ b/snapshot/mac/process_types/traits.h @@ -22,7 +22,8 @@ // DECLARE_PROCESS_TYPE_TRAITS_CLASS before #including this file again and after // the last #include of this file. // -// |Reserved*| are used for padding fields that may be zero-length, and thus +// |Reserved*| is used for padding fields that may be zero-length, and |Nothing| +// is always zero-length and is used solely as an anchor to compute offsets. // __VA_ARGS__, which is intended to set the alignment of the 64-bit types, is // not used for those type aliases. #define DECLARE_PROCESS_TYPE_TRAITS_CLASS(traits_name, lp_bits, ...) \ @@ -37,6 +38,7 @@ using UIntPtr = uint##lp_bits##_t __VA_ARGS__; \ using Reserved32_64Only = Reserved32_64Only##lp_bits; \ using Reserved64_64Only = Reserved64_64Only##lp_bits; \ + using Nothing = Nothing; \ }; \ } \ } \ diff --git a/snapshot/mac/process_types_test.cc b/snapshot/mac/process_types_test.cc index ce092fe1..8525c14e 100644 --- a/snapshot/mac/process_types_test.cc +++ b/snapshot/mac/process_types_test.cc @@ -24,6 +24,7 @@ #include "base/strings/stringprintf.h" #include "build/build_config.h" #include "gtest/gtest.h" +#include "snapshot/mac/process_types/internal.h" #include "test/mac/dyld.h" #include "util/mac/mac_util.h" #include "util/misc/implicit_cast.h" @@ -116,6 +117,42 @@ TEST(ProcessTypes, DyldImagesSelf) { process_types::dyld_all_image_infos::ExpectedSizeForVersion( &process_reader, kDyldAllImageInfosVersionInSDK)); + // Make sure that the computed sizes of various versions of this structure are + // correct at different bitnessses. + const struct { + uint32_t version; + size_t size_32; + size_t size_64; + } kVersionsAndSizes[] = { + {1, 17, 25}, + {2, 24, 40}, + {3, 28, 48}, + {5, 40, 72}, + {6, 44, 80}, + {7, 48, 88}, + {8, 56, 104}, + {9, 60, 112}, + {10, 64, 120}, + {11, 80, 152}, + {12, 84, 160}, + {13, 104, 184}, + {14, 164, 304}, + {15, 164, 304}, + }; + for (size_t index = 0; index < arraysize(kVersionsAndSizes); ++index) { + uint32_t version = kVersionsAndSizes[index].version; + SCOPED_TRACE(base::StringPrintf("index %zu, version %u", index, version)); + + EXPECT_EQ(kVersionsAndSizes[index].size_32, + process_types::internal::dyld_all_image_infos< + process_types::internal::Traits32>:: + ExpectedSizeForVersion(version)); + EXPECT_EQ(kVersionsAndSizes[index].size_64, + process_types::internal::dyld_all_image_infos< + process_types::internal::Traits64>:: + ExpectedSizeForVersion(version)); + } + process_types::dyld_all_image_infos proctype_image_infos; ASSERT_TRUE(proctype_image_infos.Read(&process_reader, dyld_info.all_image_info_addr)); From b638163e72774c9011320f98e8da7a7d454a0e13 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 7 Feb 2017 13:59:18 -0500 Subject: [PATCH 07/11] Report time zones with no DST transition within a year as not observing In locations where daylight saving time was once observed or is expected to be observed in the future, but where no transitions to or from daylight saving time occurred or will occur within a year of the current date, act as though DST is not being observed at all. Set TZ=America/Phoenix to test for this bug. BUG=crashpad:130 TEST=crashpad_snapshot_test SystemSnapshotMacTest.TimeZone Change-Id: Ie466b5906eab3c0cf2e51b962a171acb5b16210b Reviewed-on: https://chromium-review.googlesource.com/438004 Reviewed-by: Robert Sesek --- snapshot/mac/system_snapshot_mac.cc | 34 ++++++--- snapshot/mac/system_snapshot_mac_test.cc | 97 ++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 11 deletions(-) diff --git a/snapshot/mac/system_snapshot_mac.cc b/snapshot/mac/system_snapshot_mac.cc index 3c39088f..b65a9e08 100644 --- a/snapshot/mac/system_snapshot_mac.cc +++ b/snapshot/mac/system_snapshot_mac.cc @@ -19,6 +19,8 @@ #include #include +#include + #include "base/logging.h" #include "base/strings/stringprintf.h" #include "build/build_config.h" @@ -349,34 +351,44 @@ void SystemSnapshotMac::TimeZone(DaylightSavingTimeStatus* dst_status, PCHECK(localtime_r(&snapshot_time_->tv_sec, &local)) << "localtime_r"; *standard_name = tzname[0]; + + bool found_transition = false; + long probe_gmtoff = local.tm_gmtoff; if (daylight) { // Scan forward and backward, one month at a time, looking for an instance // when the observance of daylight saving time is different than it is in - // |local|. - long probe_gmtoff = local.tm_gmtoff; - + // |local|. It’s possible that no such instance will be found even with + // |daylight| set. This can happen in locations where daylight saving time + // was once observed or is expected to be observed in the future, but where + // no transitions to or from daylight saving time occurred or will occur + // within a year of the current date. Arizona, which last observed daylight + // saving time in 1967, is an example. const int kMonthDeltas[] = { 0, 1, -1, 2, -2, 3, -3, 4, -4, 5, -5, 6, -6, 7, -7, 8, -8, 9, -9, 10, -10, 11, -11, 12, -12 }; - for (size_t index = 0; index < arraysize(kMonthDeltas); ++index) { - // Look at the 15th day of each month at local noon. Set tm_isdst to -1 to - // avoid giving mktime() any hints about whether to consider daylight - // saving time in effect. mktime() accepts values of tm_mon that are - // outside of its normal range and behaves as expected: if tm_mon is -1, - // it references December of the preceding year, and if it is 12, it + for (size_t index = 0; + index < arraysize(kMonthDeltas) && !found_transition; + ++index) { + // Look at a day of each month at local noon. Set tm_isdst to -1 to avoid + // giving mktime() any hints about whether to consider daylight saving + // time in effect. mktime() accepts values of tm_mon that are outside of + // its normal range and behaves as expected: if tm_mon is -1, it + // references December of the preceding year, and if it is 12, it // references January of the following year. tm probe_tm = {}; probe_tm.tm_hour = 12; - probe_tm.tm_mday = 15; + probe_tm.tm_mday = std::min(local.tm_mday, 28); probe_tm.tm_mon = local.tm_mon + kMonthDeltas[index]; probe_tm.tm_year = local.tm_year; probe_tm.tm_isdst = -1; if (mktime(&probe_tm) != -1 && probe_tm.tm_isdst != local.tm_isdst) { + found_transition = true; probe_gmtoff = probe_tm.tm_gmtoff; - break; } } + } + if (found_transition) { *daylight_name = tzname[1]; if (!local.tm_isdst) { *dst_status = kObservingStandardTime; diff --git a/snapshot/mac/system_snapshot_mac_test.cc b/snapshot/mac/system_snapshot_mac_test.cc index a02e94e0..aff773f3 100644 --- a/snapshot/mac/system_snapshot_mac_test.cc +++ b/snapshot/mac/system_snapshot_mac_test.cc @@ -14,11 +14,13 @@ #include "snapshot/mac/system_snapshot_mac.h" +#include #include #include #include +#include "base/strings/stringprintf.h" #include "build/build_config.h" #include "gtest/gtest.h" #include "snapshot/mac/process_reader.h" @@ -127,6 +129,39 @@ TEST_F(SystemSnapshotMacTest, MachineDescription) { EXPECT_FALSE(system_snapshot().MachineDescription().empty()); } +class ScopedSetTZ { + public: + ScopedSetTZ(const std::string& tz) { + const char* old_tz = getenv(kTZ); + old_tz_set_ = old_tz; + if (old_tz_set_) { + old_tz_.assign(old_tz); + } + + EXPECT_EQ(0, setenv(kTZ, tz.c_str(), 1)) << ErrnoMessage("setenv"); + tzset(); + } + + ~ScopedSetTZ() { + if (old_tz_set_) { + EXPECT_EQ(0, setenv(kTZ, old_tz_.c_str(), 1)) << ErrnoMessage("setenv"); + } else { + EXPECT_EQ(0, unsetenv(kTZ)) << ErrnoMessage("unsetenv"); + } + tzset(); + } + + private: + std::string old_tz_; + bool old_tz_set_; + + static constexpr char kTZ[] = "TZ"; + + DISALLOW_COPY_AND_ASSIGN(ScopedSetTZ); +}; + +constexpr char ScopedSetTZ::kTZ[]; + TEST_F(SystemSnapshotMacTest, TimeZone) { SystemSnapshot::DaylightSavingTimeStatus dst_status; int standard_offset_seconds; @@ -169,6 +204,68 @@ TEST_F(SystemSnapshotMacTest, TimeZone) { EXPECT_NE(standard_name, daylight_name); } + + // Test a variety of time zones. Some of these observe daylight saving time, + // some don’t. Some used to but no longer do. Some have uncommon UTC offsets. + const struct { + const char* tz; + bool observes_dst; + float standard_offset_hours; + float daylight_offset_hours; + const char* standard_name; + const char* daylight_name; + } kTestTimeZones[] = { + {"America/Anchorage", true, -9, -8, "AKST", "AKDT"}, + {"America/Chicago", true, -6, -5, "CST", "CDT"}, + {"America/Denver", true, -7, -6, "MST", "MDT"}, + {"America/Halifax", true, -4, -3, "AST", "ADT"}, + {"America/Los_Angeles", true, -8, -7, "PST", "PDT"}, + {"America/New_York", true, -5, -4, "EST", "EDT"}, + {"America/Phoenix", false, -7, -7, "MST", "MST"}, + {"Asia/Karachi", false, 5, 5, "PKT", "PKT"}, + {"Asia/Kolkata", false, 5.5, 5.5, "IST", "IST"}, + {"Asia/Shanghai", false, 8, 8, "CST", "CST"}, + {"Asia/Tokyo", false, 9, 9, "JST", "JST"}, + {"Australia/Adelaide", true, 9.5, 10.5, "ACST", "ACDT"}, + {"Australia/Brisbane", false, 10, 10, "AEST", "AEST"}, + {"Australia/Darwin", false, 9.5, 9.5, "ACST", "ACST"}, + {"Australia/Eucla", false, 8.75, 8.75, "ACWST", "ACWST"}, + {"Australia/Lord_Howe", true, 10.5, 11, "LHST", "LHDT"}, + {"Australia/Perth", false, 8, 8, "AWST", "AWST"}, + {"Australia/Sydney", true, 10, 11, "AEST", "AEDT"}, + {"Europe/Bucharest", true, 2, 3, "EET", "EEST"}, + {"Europe/London", true, 0, 1, "GMT", "BST"}, + {"Europe/Moscow", false, 3, 3, "MSK", "MSK"}, + {"Europe/Paris", true, 1, 2, "CET", "CEST"}, + {"Europe/Reykjavik", false, 0, 0, "UTC", "UTC"}, + {"Pacific/Auckland", true, 12, 13, "NZST", "NZDT"}, + {"Pacific/Honolulu", false, -10, -10, "HST", "HST"}, + {"UTC", false, 0, 0, "UTC", "UTC"}, + }; + + for (size_t index = 0; index < arraysize(kTestTimeZones); ++index) { + const auto& test_time_zone = kTestTimeZones[index]; + const char* tz = test_time_zone.tz; + SCOPED_TRACE(base::StringPrintf("index %zu, tz %s", index, tz)); + + { + ScopedSetTZ set_tz(tz); + system_snapshot().TimeZone(&dst_status, + &standard_offset_seconds, + &daylight_offset_seconds, + &standard_name, + &daylight_name); + } + + EXPECT_EQ(test_time_zone.observes_dst, + dst_status != SystemSnapshot::kDoesNotObserveDaylightSavingTime); + EXPECT_EQ(test_time_zone.standard_offset_hours * 60 * 60, + standard_offset_seconds); + EXPECT_EQ(test_time_zone.daylight_offset_hours * 60 * 60, + daylight_offset_seconds); + EXPECT_EQ(test_time_zone.standard_name, standard_name); + EXPECT_EQ(test_time_zone.daylight_name, daylight_name); + } } } // namespace From 35020d8010007bff292783ca82d5d3155240f334 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 7 Feb 2017 14:20:30 -0500 Subject: [PATCH 08/11] Update buildtools, mini_chromium, gtest, and gyp Update buildtools to a7cc7a3e21a061975b33dcdcd81a9716ba614c3c adb8bf4e8fc9 Roll gn 4838fe571d..2eb03fab2b (r411399:r411754) 347c89790b42 Roll gn 2eb03fab2b..71c9ade4e9 (r411754:r415392) 82d2a28e425b Roll gn 2eb03fab2b..fe79dde87a (r411754:r415725) b97d6c93a3e8 Roll gn 2eb03fab2b..764c7362bc (r411754:r417994) 6115afa0ea5e Roll gn 764c7362bc..668b150d14 (r417994:r419236) f8088e3792a7 Roll gn 668b150d14..6a1c8d418d (r419236:r419720) 57649e5e2001 Roll gn 6a1c8d418d..65f3a42b24 (r419720:r419871) 86f7e41d9424 roll clang-format 258123:282138 3d2e47bf14e4 Fix repo url, remove recursion reference in DEPS 5fd66957f08b Roll gn 65f3a42b24..82dfb24218 (r419871:r421311) 39b1db2ab4aa Roll gn b6c1d4353b..bdc8e1e447 (r421341:r422996) 1f985091a586 Roll clang-format 0ed791d..6a413e9 991f459071f9 Roll gn bdc8e1e447..000b1184a0 (r422996:r432866) 102c16366d8b libc++: Don't pass -pthread to link. 64e38f0cebdd Roll gn 000b1184a0..78660e873f (r432866:r436326) 55ad626b08ef Roll gn 78660e873f..8897c835c2 (r436326:r436733) 0ef801087682 Roll gn 8897c835c2..c99acd6557 (r436733:r439377) 8932ecfa420a Roll gn c99acd6557..b1f498915e (r439377:r441559) 7e08d331f188 Roll gn c99acd6557..5c18ca83ce (r439377:r442253) 005cae407b97 Roll gn c99acd6557..7a3be23857 (r439377:r442631) 9a947138bc58 Roll gn 5c18ca83ce..7c0e0135f9 (r442253:r443802) cb12d6e8641f Roll gn 7c0e0135f9..b4dbf044c5 (r443802:r443809) 8e94621c369e Roll gn b4dbf044c5..f13158d3c5 (r443809:r445411) a7cc7a3e21a0 Roll gn f13158d3c5..d8754536ca (r445411:r446079) Update mini_chromium to e504d59673e56887a4e837cbeb44b32ec21974f9 cae485daae70 win: Initial version of toolchain for GN 57f426502e00 Enable thread-safe statics when building with GCC and clang e504d59673e5 Remove now-unused LazyInstance Update gtest to d62d6c6556d96dda924382547c54a4b3afedb22c 9759dcda3c2f Fix compilation on MinGW with native threads a138385e48ee Don't use pthread when on MinGW even if available 3429113886a9 Fix a test to compile when tuple isn't available ed9d1e1ff92c Merge pull request #721 from ilmagico/fix-mingw-threads d8fe70f477d8 Fix build with MinGW-w64 48ee8e98abc9 Merge pull request #856 from KindDragon/mingw-appveyor 10ff7f946863 Fixing relative links 16d6af7d414a Relative links 51b290d41e5d One works 9cb03aa70223 Fixing ForDummies link f5c0130e88a3 Broken relative links fixed 995db996dee6 Fixing KnownIssues and FrequentlyAskedQuestions links 960a511f45be Fixing relative links 0e0ff5c3410f blob vs tree 32b4a9b39079 Fixed broken links 8ce0b5907cd9 Cookbok: fix broken relative link ecd530865cef Merge pull request #876 from marco-m/patch-1 4eafafbde585 Fix detection of GTEST_HAS_CLONE for Android 3447fc31b4ee Merge pull request #728 from DanAlbert/tuple-stlport a2b8a8e07628 Merge pull request #918 from DanAlbert/fix-android-GTEST_HAS_CLONE cb502b7ad15c Added CMake configure-time download instructions to docs c0059a79f82d 2.6.4 is the minimum CMake version, so enforce it (#656) 5e7fd50e17b6 Merge pull request #658 from audiofanatic/ExternalProject_at_configure_time 06a81e9357b6 Add GTEST_ATTRIBUTE_UNUSED_ to REGISTER_TYPED_TEST_CASE_P 3134af23d713 Merge pull request #1 from google/master 9ae086a9ebaf Merge pull request #874 from sejr/master d62d6c6556d9 Merge pull request #982 from mbjorge/unused-variable-fix Update gyp to a7055b3989c1074adca03b4b4829e7f0e57f6efd 702ac58e4772 Add new target type called windows_driver. * Modify GYP to set the PlatformToolset, the DriverType and the TargetVersion * Add msvs_target_version configuration 5dc5a5b1718e fix common "NameError"s 920ee58c3d31 Hash intermediate file name to avoid ENAMETOOLONG ef2f29a7311b msvs: Allow target platform version without WinRT 940a15ee3f1c Update shared library extension on AIX to .a. 9733aa652da4 Set up a CQ for gyp a7055b3989c1 Make Gerrit the default code review system for gyp Change-Id: I11d8139b0f533911692dc7a11bb9edaddac78060 Reviewed-on: https://chromium-review.googlesource.com/438885 Reviewed-by: Scott Graham --- DEPS | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/DEPS b/DEPS index 77428536..8fba79f3 100644 --- a/DEPS +++ b/DEPS @@ -19,13 +19,13 @@ vars = { deps = { 'buildtools': Var('chromium_git') + '/chromium/buildtools.git@' + - 'f8fc76ea5ce4a60cda2fa5d7df3d4a62935b3113', + 'a7cc7a3e21a061975b33dcdcd81a9716ba614c3c', 'crashpad/third_party/gtest/gtest': Var('chromium_git') + '/external/github.com/google/googletest@' + - 'ec44c6c1675c25b9827aacd08c02433cccde7780', + 'd62d6c6556d96dda924382547c54a4b3afedb22c', 'crashpad/third_party/gyp/gyp': Var('chromium_git') + '/external/gyp@' + - '93cc6e2c23e4d5ebd179f388e67aa907d0dfd43d', + 'a7055b3989c1074adca03b4b4829e7f0e57f6efd', # TODO(scottmg): Consider pinning these. For now, we don't have any particular # reason to do so. @@ -38,7 +38,7 @@ deps = { 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - 'de1afb04f4afc074ec6d23bd9ee7b1e6b365427f', + 'e504d59673e56887a4e837cbeb44b32ec21974f9', } hooks = [ @@ -100,3 +100,7 @@ hooks = [ 'action': ['python', 'crashpad/build/gyp_crashpad.py'], }, ] + +recursedeps = [ + 'buildtools', +] From 88442dd5788bf7836ab013939cca4a4683560cb0 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 7 Feb 2017 16:04:41 -0500 Subject: [PATCH 09/11] Merge Chromium 294442c0ce05 upstream to Crashpad Remove stl_util from Crashpad. This also updates mini_chromium to 4f3cfc8e7c2b7d77f94f41a32c3ec84a6920f05d to remove stl_util from there as well. 4f3cfc8e7c2b Remove stl_util from mini_chromium BUG=chromium:555865 Change-Id: I8ecb1639a258dd233d524834ed205a4fcc641bac Reviewed-on: https://chromium-review.googlesource.com/438865 Reviewed-by: Scott Graham --- DEPS | 2 +- minidump/minidump_handle_writer.cc | 4 +-- ...inidump_simple_string_dictionary_writer.cc | 4 +-- snapshot/win/thread_snapshot_win.cc | 1 + util/net/http_body.cc | 4 +-- util/stdlib/pointer_container.h | 34 +++++++------------ 6 files changed, 20 insertions(+), 29 deletions(-) diff --git a/DEPS b/DEPS index 8fba79f3..db8c048c 100644 --- a/DEPS +++ b/DEPS @@ -38,7 +38,7 @@ deps = { 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - 'e504d59673e56887a4e837cbeb44b32ec21974f9', + '4f3cfc8e7c2b7d77f94f41a32c3ec84a6920f05d', } hooks = [ diff --git a/minidump/minidump_handle_writer.cc b/minidump/minidump_handle_writer.cc index c5d4659f..098b40a1 100644 --- a/minidump/minidump_handle_writer.cc +++ b/minidump/minidump_handle_writer.cc @@ -17,7 +17,6 @@ #include #include "base/logging.h" -#include "base/stl_util.h" #include "minidump/minidump_extensions.h" #include "util/file/file_writer.h" #include "util/numeric/safe_assignment.h" @@ -29,7 +28,8 @@ MinidumpHandleDataWriter::MinidumpHandleDataWriter() } MinidumpHandleDataWriter::~MinidumpHandleDataWriter() { - base::STLDeleteContainerPairSecondPointers(strings_.begin(), strings_.end()); + for (auto& item : strings_) + delete item.second; } void MinidumpHandleDataWriter::InitializeFromSnapshot( diff --git a/minidump/minidump_simple_string_dictionary_writer.cc b/minidump/minidump_simple_string_dictionary_writer.cc index b4dd722e..ae8bfaba 100644 --- a/minidump/minidump_simple_string_dictionary_writer.cc +++ b/minidump/minidump_simple_string_dictionary_writer.cc @@ -18,7 +18,6 @@ #include "base/logging.h" #include "base/memory/ptr_util.h" -#include "base/stl_util.h" #include "util/file/file_writer.h" #include "util/numeric/safe_assignment.h" @@ -100,7 +99,8 @@ MinidumpSimpleStringDictionaryWriter::MinidumpSimpleStringDictionaryWriter() } MinidumpSimpleStringDictionaryWriter::~MinidumpSimpleStringDictionaryWriter() { - base::STLDeleteContainerPairSecondPointers(entries_.begin(), entries_.end()); + for (auto& item : entries_) + delete item.second; } void MinidumpSimpleStringDictionaryWriter::InitializeFromMap( diff --git a/snapshot/win/thread_snapshot_win.cc b/snapshot/win/thread_snapshot_win.cc index 8b86730c..d737c14b 100644 --- a/snapshot/win/thread_snapshot_win.cc +++ b/snapshot/win/thread_snapshot_win.cc @@ -14,6 +14,7 @@ #include "snapshot/win/thread_snapshot_win.h" +#include #include #include "base/logging.h" diff --git a/util/net/http_body.cc b/util/net/http_body.cc index b1bca19e..96a7cc40 100644 --- a/util/net/http_body.cc +++ b/util/net/http_body.cc @@ -20,7 +20,6 @@ #include #include "base/logging.h" -#include "base/stl_util.h" #include "util/misc/implicit_cast.h" namespace crashpad { @@ -89,7 +88,8 @@ CompositeHTTPBodyStream::CompositeHTTPBodyStream( } CompositeHTTPBodyStream::~CompositeHTTPBodyStream() { - base::STLDeleteContainerPointers(parts_.begin(), parts_.end()); + for (auto& item : parts_) + delete item; } FileOperationResult CompositeHTTPBodyStream::GetBytesBuffer(uint8_t* buffer, diff --git a/util/stdlib/pointer_container.h b/util/stdlib/pointer_container.h index 576270df..7d2d5d06 100644 --- a/util/stdlib/pointer_container.h +++ b/util/stdlib/pointer_container.h @@ -18,30 +18,9 @@ #include #include "base/macros.h" -#include "base/stl_util.h" namespace crashpad { -//! \brief Allows a standard container to “own” pointer elements stored in it. -//! -//! When the container is destroyed, `delete` will be called on its pointer -//! elements. -//! -//! \note No attempt is made to `delete` elements that are removed from the -//! container by other means, such as replacement or `clear()`. -template -class PointerContainer : public ContainerType { - public: - PointerContainer() : ContainerType(), pointer_deleter_(this) {} - - ~PointerContainer() {} - - private: - base::STLElementDeleter pointer_deleter_; - - DISALLOW_COPY_AND_ASSIGN(PointerContainer); -}; - //! \brief Allows a `std::vector` to “own” pointer elements stored in it. //! //! When the vector is destroyed, `delete` will be called on its pointer @@ -50,7 +29,18 @@ class PointerContainer : public ContainerType { //! \note No attempt is made to `delete` elements that are removed from the //! vector by other means, such as replacement or `clear()`. template -using PointerVector = PointerContainer>; +class PointerVector : public std::vector { + public: + PointerVector() : std::vector() {} + + ~PointerVector() { + for (T* item : *this) + delete item; + } + + private: + DISALLOW_COPY_AND_ASSIGN(PointerVector); +}; } // namespace crashpad From 6af23a933a0dc10cda072570cc02efcd03488a90 Mon Sep 17 00:00:00 2001 From: Sigurdur Asgeirsson Date: Wed, 8 Feb 2017 10:38:09 -0500 Subject: [PATCH 10/11] Use best-effort allocation in ProcessInfo::BuildHandleVector. BUG=crashpad:158 Change-Id: If8666140a7fc5315eeb791d0998226de89a22cc3 Reviewed-on: https://chromium-review.googlesource.com/438791 Reviewed-by: Mark Mentovai Reviewed-by: Scott Graham --- DEPS | 2 +- util/win/process_info.cc | 25 +++++++++++++++++++++---- util/win/process_info.h | 1 + 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/DEPS b/DEPS index db8c048c..0da988b7 100644 --- a/DEPS +++ b/DEPS @@ -38,7 +38,7 @@ deps = { 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - '4f3cfc8e7c2b7d77f94f41a32c3ec84a6920f05d', + 'f65519e442d23498937251e680a3b113927613b0', } hooks = [ diff --git a/util/win/process_info.cc b/util/win/process_info.cc index d05a23d1..2e285b8b 100644 --- a/util/win/process_info.cc +++ b/util/win/process_info.cc @@ -19,9 +19,12 @@ #include #include #include +#include #include #include "base/logging.h" +#include "base/memory/free_deleter.h" +#include "base/process/memory.h" #include "base/strings/stringprintf.h" #include "build/build_config.h" #include "util/numeric/safe_assignment.h" @@ -36,6 +39,16 @@ namespace crashpad { namespace { +using UniqueMallocPtr = std::unique_ptr; + +UniqueMallocPtr UncheckedAllocate(size_t size) { + void* raw_ptr = nullptr; + if (!base::UncheckedMalloc(size, &raw_ptr)) + return UniqueMallocPtr(); + + return UniqueMallocPtr(new (raw_ptr) uint8_t[size]); +} + NTSTATUS NtQueryInformationProcess(HANDLE process_handle, PROCESSINFOCLASS process_information_class, PVOID process_information, @@ -347,14 +360,20 @@ bool ReadMemoryInfo(HANDLE process, bool is_64_bit, ProcessInfo* process_info) { std::vector ProcessInfo::BuildHandleVector( HANDLE process) const { ULONG buffer_size = 2 * 1024 * 1024; - std::unique_ptr buffer(new uint8_t[buffer_size]); - // Typically if the buffer were too small, STATUS_INFO_LENGTH_MISMATCH would // return the correct size in the final argument, but it does not for // SystemExtendedHandleInformation, so we loop and attempt larger sizes. NTSTATUS status; ULONG returned_length; + UniqueMallocPtr buffer; for (int tries = 0; tries < 5; ++tries) { + buffer.reset(); + buffer = UncheckedAllocate(buffer_size); + if (!buffer) { + LOG(ERROR) << "UncheckedAllocate"; + return std::vector(); + } + status = crashpad::NtQuerySystemInformation( static_cast(SystemExtendedHandleInformation), buffer.get(), @@ -364,8 +383,6 @@ std::vector ProcessInfo::BuildHandleVector( break; buffer_size *= 2; - buffer.reset(); - buffer.reset(new uint8_t[buffer_size]); } if (!NT_SUCCESS(status)) { diff --git a/util/win/process_info.h b/util/win/process_info.h index 0bb8d735..be968f92 100644 --- a/util/win/process_info.h +++ b/util/win/process_info.h @@ -170,6 +170,7 @@ class ProcessInfo { bool is_64_bit, ProcessInfo* process_info); + // This function is best-effort under low memory conditions. std::vector BuildHandleVector(HANDLE process) const; pid_t process_id_; From cd284713834e4d9c184dd06643b0a87f73d9b062 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 8 Feb 2017 16:16:06 -0500 Subject: [PATCH 11/11] win: Implement Transfer-Encoding: chunked for HTTP requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chunked encoding doesn’t require the length of the request body to be known in advance. In cases where this value isn’t independently known, as is normal for Crashpad report uploads where the HTTP request body is constructed on the fly, chunked encoding eliminates the need to prepare the entire request body in memory before transmitting it. In these cases, it’s much less wasteful. When the length of the request body is known in advance, based on the provision of a Content-Length header, chunked encoding is not used. Even so, the request is sent in pieces rather than reading the entire request into memory before sending anything. BUG=crashpad:159 TEST=crashpad_util_test HTTPTransport.* Change-Id: Iebb2b63b936065cb8c3c4a62b58f9c14fec43937 Reviewed-on: https://chromium-review.googlesource.com/439644 Reviewed-by: Scott Graham --- util/net/http_transport_test_server.py | 8 +- util/net/http_transport_win.cc | 194 +++++++++++++++++++------ 2 files changed, 152 insertions(+), 50 deletions(-) diff --git a/util/net/http_transport_test_server.py b/util/net/http_transport_test_server.py index 3f085a56..3d3c55a4 100755 --- a/util/net/http_transport_test_server.py +++ b/util/net/http_transport_test_server.py @@ -26,7 +26,7 @@ process one HTTP request, deliver the prearranged response to the client, and write the entire request to stdout. It will then terminate. This server is written in Python since it provides a simple HTTP stack, and -because parsing Chunked encoding is safer and easier in a memory-safe language. +because parsing chunked encoding is safer and easier in a memory-safe language. This could easily have been written in C++ instead. """ @@ -80,7 +80,7 @@ class RequestHandler(BaseHTTPServer.BaseHTTPRequestHandler): RequestHandler.raw_request = self.rfile.buffer self.rfile.buffer = '' - if self.headers.get('Transfer-Encoding', '') == 'Chunked': + if self.headers.get('Transfer-Encoding', '').lower() == 'chunked': body = self.handle_chunked_encoding() else: length = int(self.headers.get('Content-Length', -1)) @@ -130,6 +130,10 @@ class RequestHandler(BaseHTTPServer.BaseHTTPRequestHandler): return -1 return int(chunk_size_and_ext_line[:chunk_size_end], base=16) + def log_request(self, code='-', size='-'): + # The default implementation logs these to sys.stderr, which is just noise. + pass + def Main(): if sys.platform == 'win32': diff --git a/util/net/http_transport_win.cc b/util/net/http_transport_win.cc index 58ecc478..294048a9 100644 --- a/util/net/http_transport_win.cc +++ b/util/net/http_transport_win.cc @@ -15,17 +15,22 @@ #include "util/net/http_transport.h" #include +#include #include +#include #include +#include #include #include "base/logging.h" #include "base/numerics/safe_conversions.h" #include "base/scoped_generic.h" +#include "base/strings/string_number_conversions.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" #include "package.h" #include "util/file/file_io.h" +#include "util/numeric/safe_assignment.h" #include "util/net/http_body.h" namespace crashpad { @@ -34,7 +39,7 @@ namespace { // PLOG doesn't work for messages from WinHTTP, so we need to use // FORMAT_MESSAGE_FROM_HMODULE + the dll name manually here. -void LogErrorWinHttpMessage(const char* extra) { +std::string WinHttpMessage(const char* extra) { DWORD error_code = GetLastError(); char msgbuf[256]; DWORD flags = FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS | @@ -46,15 +51,13 @@ void LogErrorWinHttpMessage(const char* extra) { msgbuf, arraysize(msgbuf), NULL); - if (len) { - LOG(ERROR) << extra << ": " << msgbuf - << base::StringPrintf(" (0x%X)", error_code); - } else { - LOG(ERROR) << base::StringPrintf( - "Error (0x%X) while retrieving error. (0x%X)", - GetLastError(), - error_code); + if (!len) { + return base::StringPrintf("%s: error 0x%x while retrieving error 0x%x", + extra, + GetLastError(), + error_code); } + return base::StringPrintf("%s: %s (0x%x)", extra, msgbuf, error_code); } struct ScopedHINTERNETTraits { @@ -64,7 +67,7 @@ struct ScopedHINTERNETTraits { static void Free(HINTERNET handle) { if (handle) { if (!WinHttpCloseHandle(handle)) { - LogErrorWinHttpMessage("WinHttpCloseHandle"); + LOG(ERROR) << WinHttpMessage("WinHttpCloseHandle"); } } } @@ -97,7 +100,7 @@ bool HTTPTransportWin::ExecuteSynchronously(std::string* response_body) { WINHTTP_NO_PROXY_BYPASS, 0)); if (!session.get()) { - LogErrorWinHttpMessage("WinHttpOpen"); + LOG(ERROR) << WinHttpMessage("WinHttpOpen"); return false; } @@ -107,7 +110,7 @@ bool HTTPTransportWin::ExecuteSynchronously(std::string* response_body) { timeout_in_ms, timeout_in_ms, timeout_in_ms)) { - LogErrorWinHttpMessage("WinHttpSetTimeouts"); + LOG(ERROR) << WinHttpMessage("WinHttpSetTimeouts"); return false; } @@ -121,7 +124,7 @@ bool HTTPTransportWin::ExecuteSynchronously(std::string* response_body) { // https://msdn.microsoft.com/en-us/library/aa384092.aspx if (!WinHttpCrackUrl( url_wide.c_str(), 0, 0, &url_components)) { - LogErrorWinHttpMessage("WinHttpCrackUrl"); + LOG(ERROR) << WinHttpMessage("WinHttpCrackUrl"); return false; } DCHECK(url_components.nScheme == INTERNET_SCHEME_HTTP || @@ -136,7 +139,7 @@ bool HTTPTransportWin::ExecuteSynchronously(std::string* response_body) { ScopedHINTERNET connect(WinHttpConnect( session.get(), host_name.c_str(), url_components.nPort, 0)); if (!connect.get()) { - LogErrorWinHttpMessage("WinHttpConnect"); + LOG(ERROR) << WinHttpMessage("WinHttpConnect"); return false; } @@ -150,57 +153,152 @@ bool HTTPTransportWin::ExecuteSynchronously(std::string* response_body) { url_components.nScheme == INTERNET_SCHEME_HTTPS ? WINHTTP_FLAG_SECURE : 0)); if (!request.get()) { - LogErrorWinHttpMessage("WinHttpOpenRequest"); + LOG(ERROR) << WinHttpMessage("WinHttpOpenRequest"); return false; } // Add headers to the request. + // + // If Content-Length is not provided, implement chunked mode per RFC 7230 + // §4.1. + // + // Note that chunked mode can only be used on Vista and later. Otherwise, + // WinHttpSendRequest() requires a real value for dwTotalLength, used for the + // Content-Length header. Determining that in the absence of a provided + // Content-Length would require reading the entire request body before calling + // WinHttpSendRequest(). + bool chunked = true; + size_t content_length = 0; for (const auto& pair : headers()) { - std::wstring header_string = - base::UTF8ToUTF16(pair.first) + L": " + base::UTF8ToUTF16(pair.second); - if (!WinHttpAddRequestHeaders( - request.get(), - header_string.c_str(), - base::checked_cast(header_string.size()), - WINHTTP_ADDREQ_FLAG_ADD)) { - LogErrorWinHttpMessage("WinHttpAddRequestHeaders"); - return false; + if (pair.first == kContentLength) { + chunked = !base::StringToSizeT(pair.second, &content_length); + DCHECK(!chunked); + } else { + std::wstring header_string = base::UTF8ToUTF16(pair.first) + L": " + + base::UTF8ToUTF16(pair.second) + L"\r\n"; + if (!WinHttpAddRequestHeaders( + request.get(), + header_string.c_str(), + base::checked_cast(header_string.size()), + WINHTTP_ADDREQ_FLAG_ADD)) { + LOG(ERROR) << WinHttpMessage("WinHttpAddRequestHeaders"); + return false; + } } } - // We need the Content-Length up front, so buffer in memory. We should modify - // the interface to not require this, and then use WinHttpWriteData after - // WinHttpSendRequest. - std::vector post_data; - - // Write the body of a POST if any. - const size_t kBufferSize = 4096; - for (;;) { - uint8_t buffer[kBufferSize]; - FileOperationResult bytes_to_write = - body_stream()->GetBytesBuffer(buffer, sizeof(buffer)); - if (bytes_to_write == 0) { - break; - } else if (bytes_to_write < 0) { - LOG(ERROR) << "GetBytesBuffer failed"; + DWORD content_length_dword; + if (chunked) { + const wchar_t kTransferEncodingHeader[] = L"Transfer-Encoding: chunked\r\n"; + if (!WinHttpAddRequestHeaders( + request.get(), + kTransferEncodingHeader, + base::checked_cast(wcslen(kTransferEncodingHeader)), + WINHTTP_ADDREQ_FLAG_ADD)) { + LOG(ERROR) << WinHttpMessage("WinHttpAddRequestHeaders"); return false; } - post_data.insert(post_data.end(), buffer, buffer + bytes_to_write); + + content_length_dword = WINHTTP_IGNORE_REQUEST_TOTAL_LENGTH; + } else if (!AssignIfInRange(&content_length_dword, content_length)) { + content_length_dword = WINHTTP_IGNORE_REQUEST_TOTAL_LENGTH; } if (!WinHttpSendRequest(request.get(), WINHTTP_NO_ADDITIONAL_HEADERS, 0, - &post_data[0], - base::checked_cast(post_data.size()), - base::checked_cast(post_data.size()), + WINHTTP_NO_REQUEST_DATA, + 0, + content_length_dword, 0)) { - LogErrorWinHttpMessage("WinHttpSendRequest"); + LOG(ERROR) << WinHttpMessage("WinHttpSendRequest"); return false; } + size_t total_written = 0; + FileOperationResult data_bytes; + do { + struct { + char size[8]; + char crlf0[2]; + uint8_t data[32 * 1024]; + char crlf1[2]; + } buf; + static_assert(sizeof(buf) == sizeof(buf.size) + + sizeof(buf.crlf0) + + sizeof(buf.data) + + sizeof(buf.crlf1), + "buf should not have padding"); + + // Read a block of data. + data_bytes = body_stream()->GetBytesBuffer(buf.data, sizeof(buf.data)); + if (data_bytes == -1) { + return false; + } + DCHECK_GE(data_bytes, 0); + DCHECK_LE(static_cast(data_bytes), sizeof(buf.data)); + + void* write_start; + DWORD write_size; + + if (chunked) { + // Chunked encoding uses the entirety of buf. buf.size is presented in + // hexadecimal without any leading "0x". The terminating CR and LF will be + // placed immediately following the used portion of buf.data, even if + // buf.data is not full, and not necessarily in buf.crlf1. + + unsigned int data_bytes_ui = base::checked_cast(data_bytes); + + // snprintf() would NUL-terminate, but _snprintf() won’t. + int rv = _snprintf(buf.size, sizeof(buf.size), "%08x", data_bytes_ui); + DCHECK_GE(rv, 0); + DCHECK_EQ(static_cast(rv), sizeof(buf.size)); + DCHECK_NE(buf.size[sizeof(buf.size) - 1], '\0'); + + buf.crlf0[0] = '\r'; + buf.crlf0[1] = '\n'; + buf.data[data_bytes] = '\r'; + buf.data[data_bytes + 1] = '\n'; + + // Skip leading zeroes in the chunk size. + unsigned int size_len; + for (size_len = sizeof(buf.size); size_len > 1; --size_len) { + if (buf.size[sizeof(buf.size) - size_len] != '0') { + break; + } + } + + write_start = buf.crlf0 - size_len; + write_size = base::checked_cast(size_len + sizeof(buf.crlf0) + + data_bytes + sizeof(buf.crlf1)); + } else { + // When not using chunked encoding, only use buf.data. + write_start = buf.data; + write_size = base::checked_cast(data_bytes); + } + + // write_size will be 0 at EOF in non-chunked mode. Skip the write in that + // case. In contrast, at EOF in chunked mode, a zero-length chunk must be + // sent to signal EOF. This will happen when processing the EOF indicated by + // a 0 return from body_stream()->GetBytesBuffer() above. + if (write_size != 0) { + DWORD written; + if (!WinHttpWriteData(request.get(), write_start, write_size, &written)) { + LOG(ERROR) << WinHttpMessage("WinHttpWriteData"); + return false; + } + + DCHECK_EQ(written, write_size); + total_written += written; + } + } while (data_bytes > 0); + + if (!chunked) { + DCHECK_EQ(total_written, content_length); + } + if (!WinHttpReceiveResponse(request.get(), nullptr)) { - LogErrorWinHttpMessage("WinHttpReceiveResponse"); + LOG(ERROR) << WinHttpMessage("WinHttpReceiveResponse"); return false; } @@ -214,7 +312,7 @@ bool HTTPTransportWin::ExecuteSynchronously(std::string* response_body) { &status_code, &sizeof_status_code, WINHTTP_NO_HEADER_INDEX)) { - LogErrorWinHttpMessage("WinHttpQueryHeaders"); + LOG(ERROR) << WinHttpMessage("WinHttpQueryHeaders"); return false; } @@ -232,10 +330,10 @@ bool HTTPTransportWin::ExecuteSynchronously(std::string* response_body) { // which executes synchronously, is only concerned with reading until EOF. DWORD bytes_read = 0; do { - char read_buffer[kBufferSize]; + char read_buffer[4096]; if (!WinHttpReadData( request.get(), read_buffer, sizeof(read_buffer), &bytes_read)) { - LogErrorWinHttpMessage("WinHttpReadData"); + LOG(ERROR) << WinHttpMessage("WinHttpReadData"); return false; }