From 72fbc56e58d3ab97ab7184b2636706412a458df6 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 11 Nov 2016 12:49:58 -0500 Subject: [PATCH 01/19] =?UTF-8?q?MSVC++=20fix:=20Don=E2=80=99t=20declare?= =?UTF-8?q?=20local[0]=20arrays=20for=20ARRAYSIZE=5FUNSAFE=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After f83530bf9a0b, while compiling arraysize_unsafe_test.cc: …\crashpad\util\misc\arraysize_unsafe_test.cc(24) : error C2466: cannot allocate an array of constant size 0 …\crashpad\util\misc\arraysize_unsafe_test.cc(24) : error C2133: 'c0' : unknown size …\crashpad\util\misc\arraysize_unsafe_test.cc(25) : error C2070: 'char []': illegal sizeof operand …\crashpad\util\misc\arraysize_unsafe_test.cc(36) : error C2466: cannot allocate an array of constant size 0 …\crashpad\util\misc\arraysize_unsafe_test.cc(36) : error C2133: 'i0' : unknown size …\crashpad\util\misc\arraysize_unsafe_test.cc(37) : error C2070: 'int []': illegal sizeof operand …\crashpad\util\misc\arraysize_unsafe_test.cc(61) : error C2466: cannot allocate an array of constant size 0 …\crashpad\util\misc\arraysize_unsafe_test.cc(61) : error C2133: 's0' : unknown size …\crashpad\util\misc\arraysize_unsafe_test.cc(62) : error C2070: 'crashpad::test::`anonymous-namespace'::ArraySizeUnsafe_ArraySizeUnsafe_Test::TestBody::S []': illegal sizeof operand MSVC++ 2015 (14.0) doesn’t mind, and I thought that testing that version would be enough, but the Crashpad buildbots still run MSVC++ 2013 (12.0), which doesn’t like this construct. https://build.chromium.org/p/client.crashpad/builders/crashpad_win_x64_dbg/builds/265/steps/compile%20with%20ninja/logs/stdio TBR=scottmg@chromium.org (holiday) Change-Id: Ia8d140ceda3cd1bdec09c78560377b9bfad84dc4 Reviewed-on: https://chromium-review.googlesource.com/410128 Reviewed-by: Mark Mentovai --- util/misc/arraysize_unsafe_test.cc | 9 --------- 1 file changed, 9 deletions(-) diff --git a/util/misc/arraysize_unsafe_test.cc b/util/misc/arraysize_unsafe_test.cc index 1c638e02..ed2e6abe 100644 --- a/util/misc/arraysize_unsafe_test.cc +++ b/util/misc/arraysize_unsafe_test.cc @@ -21,9 +21,6 @@ namespace test { namespace { TEST(ArraySizeUnsafe, ArraySizeUnsafe) { - char c0[0]; - static_assert(ARRAYSIZE_UNSAFE(c0) == 0, "c0"); - char c1[1]; static_assert(ARRAYSIZE_UNSAFE(c1) == 1, "c1"); @@ -33,9 +30,6 @@ TEST(ArraySizeUnsafe, ArraySizeUnsafe) { char c4[4]; static_assert(ARRAYSIZE_UNSAFE(c4) == 4, "c4"); - int i0[0]; - static_assert(ARRAYSIZE_UNSAFE(i0) == 0, "i0"); - int i1[1]; static_assert(ARRAYSIZE_UNSAFE(i1) == 1, "i1"); @@ -58,9 +52,6 @@ TEST(ArraySizeUnsafe, ArraySizeUnsafe) { bool b; }; - S s0[0]; - static_assert(ARRAYSIZE_UNSAFE(s0) == 0, "s0"); - S s1[1]; static_assert(ARRAYSIZE_UNSAFE(s1) == 1, "s1"); From b37aa95da79c01aa57173ddccea93d461469f38a Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 11 Nov 2016 13:06:30 -0500 Subject: [PATCH 02/19] MSVC++ fix: ALLOW_UNUSED_LOCAL variables only used in static_assert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After f83530bf9a0b and 72fbc56e58d3, while compiling arraysize_unsafe_test.cc: …\crashpad\util\misc\arraysize_unsafe_test.cc(58) : error C2220: warning treated as error - no 'object' file generated …\crashpad\util\misc\arraysize_unsafe_test.cc(58) : warning C4101: 's10' : unreferenced local variable …\crashpad\util\misc\arraysize_unsafe_test.cc(33) : warning C4101: 'i1' : unreferenced local variable …\crashpad\util\misc\arraysize_unsafe_test.cc(24) : warning C4101: 'c1' : unreferenced local variable …\crashpad\util\misc\arraysize_unsafe_test.cc(27) : warning C4101: 'c2' : unreferenced local variable …\crashpad\util\misc\arraysize_unsafe_test.cc(55) : warning C4101: 's1' : unreferenced local variable …\crashpad\util\misc\arraysize_unsafe_test.cc(39) : warning C4101: 'i4' : unreferenced local variable …\crashpad\util\misc\arraysize_unsafe_test.cc(45) : warning C4101: 'l9' : unreferenced local variable …\crashpad\util\misc\arraysize_unsafe_test.cc(30) : warning C4101: 'c4' : unreferenced local variable …\crashpad\util\misc\arraysize_unsafe_test.cc(42) : warning C4101: 'l8' : unreferenced local variable …\crashpad\util\misc\arraysize_unsafe_test.cc(36) : warning C4101: 'i2' : unreferenced local variable The line numbers are totally out of order! I think that my error was not actually ever running “gclient runhooks”, so I never tested this locally on Windows as I thought I had. https://build.chromium.org/p/client.crashpad/builders/crashpad_win_x64_dbg/builds/266/steps/compile%20with%20ninja/logs/stdio TBR=scottmg@chromium.org (holiday) Change-Id: I00414b54c04b5b7e3aa564b0c6fd49d20a47b6ea Reviewed-on: https://chromium-review.googlesource.com/410129 Reviewed-by: Mark Mentovai --- util/misc/arraysize_unsafe_test.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/util/misc/arraysize_unsafe_test.cc b/util/misc/arraysize_unsafe_test.cc index ed2e6abe..a6660aee 100644 --- a/util/misc/arraysize_unsafe_test.cc +++ b/util/misc/arraysize_unsafe_test.cc @@ -14,6 +14,7 @@ #include "util/misc/arraysize_unsafe.h" +#include "base/compiler_specific.h" #include "gtest/gtest.h" namespace crashpad { @@ -23,27 +24,35 @@ namespace { TEST(ArraySizeUnsafe, ArraySizeUnsafe) { char c1[1]; static_assert(ARRAYSIZE_UNSAFE(c1) == 1, "c1"); + ALLOW_UNUSED_LOCAL(c1); char c2[2]; static_assert(ARRAYSIZE_UNSAFE(c2) == 2, "c2"); + ALLOW_UNUSED_LOCAL(c2); char c4[4]; static_assert(ARRAYSIZE_UNSAFE(c4) == 4, "c4"); + ALLOW_UNUSED_LOCAL(c4); int i1[1]; static_assert(ARRAYSIZE_UNSAFE(i1) == 1, "i1"); + ALLOW_UNUSED_LOCAL(i1); int i2[2]; static_assert(ARRAYSIZE_UNSAFE(i2) == 2, "i2"); + ALLOW_UNUSED_LOCAL(i2); int i4[4]; static_assert(ARRAYSIZE_UNSAFE(i4) == 4, "i4"); + ALLOW_UNUSED_LOCAL(i4); long l8[8]; static_assert(ARRAYSIZE_UNSAFE(l8) == 8, "l8"); + ALLOW_UNUSED_LOCAL(l8); int l9[9]; static_assert(ARRAYSIZE_UNSAFE(l9) == 9, "l9"); + ALLOW_UNUSED_LOCAL(l9); struct S { char c; @@ -54,9 +63,11 @@ TEST(ArraySizeUnsafe, ArraySizeUnsafe) { S s1[1]; static_assert(ARRAYSIZE_UNSAFE(s1) == 1, "s1"); + ALLOW_UNUSED_LOCAL(s1); S s10[10]; static_assert(ARRAYSIZE_UNSAFE(s10) == 10, "s10"); + ALLOW_UNUSED_LOCAL(s10); } } // namespace From f09d0cde00a1038d08f59eab7cb6d72cf596a09d Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 15 Nov 2016 13:50:32 -0500 Subject: [PATCH 03/19] Improve time handling and error checking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The database settings object’s last_upload_attempt_time (time_t) field is switched from uint64_t to int64_t, for better compatibility with time_t, which is normally a signed type. This change should be transparent, as there should be no valid high-bit-set 64-bit timestamps in this field in the wild. A number of improvements are made to crashpad_database_util’s time handling. Errors are checked during time conversion. --set-last-upload-attempt-time=now is a new supported (and documented) option. A StringToNumber() overload for int64_t, along with a test, is added to aid in crashpad_database_util’s time conversions from numeric strings. A test is also added for the previously-untested uint64_t implementation. TEST=crashpad_util_test StringNumberConversion.* Change-Id: I089c4bf7b95f5df0982bdbb3c27b4f6a89db966e Reviewed-on: https://chromium-review.googlesource.com/410068 Reviewed-by: Robert Sesek --- client/settings.cc | 4 +- tools/crashpad_database_util.cc | 67 ++++++++----- tools/crashpad_database_util.md | 3 +- util/stdlib/string_number_conversion.cc | 11 ++ util/stdlib/string_number_conversion.h | 1 + util/stdlib/string_number_conversion_test.cc | 100 +++++++++++++++++++ 6 files changed, 160 insertions(+), 26 deletions(-) diff --git a/client/settings.cc b/client/settings.cc index d018e37f..7757ecb0 100644 --- a/client/settings.cc +++ b/client/settings.cc @@ -55,7 +55,7 @@ struct Settings::Data { uint32_t version; uint32_t options; uint32_t padding_0; - uint64_t last_upload_attempt_time; // time_t + int64_t last_upload_attempt_time; // time_t UUID client_id; }; @@ -136,7 +136,7 @@ bool Settings::SetLastUploadAttemptTime(time_t time) { if (!handle.is_valid()) return false; - settings.last_upload_attempt_time = InRangeCast(time, 0); + settings.last_upload_attempt_time = InRangeCast(time, 0); return WriteSettings(handle.get(), settings); } diff --git a/tools/crashpad_database_util.cc b/tools/crashpad_database_util.cc index acbadd02..a9aeb72b 100644 --- a/tools/crashpad_database_util.cc +++ b/tools/crashpad_database_util.cc @@ -12,12 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include #include #include #include #include #include +#include #include #include @@ -38,6 +38,7 @@ #include "util/file/file_io.h" #include "util/file/file_reader.h" #include "util/misc/uuid.h" +#include "util/stdlib/string_number_conversion.h" namespace crashpad { namespace { @@ -130,16 +131,24 @@ std::string BoolToString(bool boolean) { return std::string(boolean ? "true" : "false"); } -// Converts |string| to |time|, returning true if a conversion could be +// Converts |string| to |out_time|, returning true if a conversion could be // performed, and false without setting |boolean| if no conversion could be // performed. Various time formats are recognized, including several string -// representations and a numeric time_t representation. The special string -// "never" is recognized as |string| and converts to a |time| value of 0. |utc|, -// when true, causes |string| to be interpreted as a UTC time rather than a -// local time when the time zone is ambiguous. -bool StringToTime(const char* string, time_t* time, bool utc) { +// representations and a numeric time_t representation. The special |string| +// "never" is recognized as converted to a |out_time| value of 0; "now" is +// converted to the current time. |utc|, when true, causes |string| to be +// interpreted as a UTC time rather than a local time when the time zone is +// ambiguous. +bool StringToTime(const char* string, time_t* out_time, bool utc) { if (strcasecmp(string, "never") == 0) { - *time = 0; + *out_time = 0; + return true; + } + + if (strcasecmp(string, "now") == 0) { + timeval tv; + PCHECK(gettimeofday(&tv, nullptr) == 0); + *out_time = tv.tv_sec; return true; } @@ -155,41 +164,53 @@ bool StringToTime(const char* string, time_t* time, bool utc) { tm time_tm; const char* strptime_result = strptime(string, kFormats[index], &time_tm); if (strptime_result == end) { + time_t test_out_time; if (utc) { - *time = timegm(&time_tm); + test_out_time = timegm(&time_tm); } else { - *time = mktime(&time_tm); + test_out_time = mktime(&time_tm); } - return true; + // mktime() is supposed to set errno in the event of an error, but support + // for this is spotty, so there’s no way to distinguish between a true + // time_t of -1 (1969-12-31 23:59:59 UTC) and an error. Assume error. + // + // See 10.11.5 Libc-1082.50.1/stdtime/FreeBSD/localtime.c and + // glibc-2.24/time/mktime.c, which don’t set errno or save and restore + // errno. Post-Android 7.1.0 Bionic is even more hopeless, setting errno + // whenever the time conversion returns -1, even for valid input. See + // libc/tzcode/localtime.c mktime(). Windows seems to get it right: see + // 10.0.14393 SDK Source/ucrt/time/mktime.cpp. + if (test_out_time != -1) { + *out_time = test_out_time; + return true; + } } } - char* end_result; - errno = 0; - long long strtoll_result = strtoll(string, &end_result, 0); - if (end_result == end && errno == 0 && - base::IsValueInRangeForNumericType(strtoll_result)) { - *time = strtoll_result; + int64_t int64_result; + if (StringToNumber(string, &int64_result) && + base::IsValueInRangeForNumericType(int64_result)) { + *out_time = int64_result; return true; } return false; } -// Converts |time_tt| to a string, and returns it. |utc| determines whether the -// converted time will reference local time or UTC. If |time_tt| is 0, the +// Converts |out_time| to a string, and returns it. |utc| determines whether the +// converted time will reference local time or UTC. If |out_time| is 0, the // string "never" will be returned as a special case. -std::string TimeToString(time_t time_tt, bool utc) { - if (time_tt == 0) { +std::string TimeToString(time_t out_time, bool utc) { + if (out_time == 0) { return std::string("never"); } tm time_tm; if (utc) { - gmtime_r(&time_tt, &time_tm); + PCHECK(gmtime_r(&out_time, &time_tm)); } else { - localtime_r(&time_tt, &time_tm); + PCHECK(localtime_r(&out_time, &time_tm)); } char string[64]; diff --git a/tools/crashpad_database_util.md b/tools/crashpad_database_util.md index 610731dd..a63d9a68 100644 --- a/tools/crashpad_database_util.md +++ b/tools/crashpad_database_util.md @@ -118,7 +118,8 @@ database. Set the last-upload-attempt time in the database’s settings. _TIME_ is a string representation of a time, which may be in _yyyy-mm-dd hh:mm:ss_ - format, a numeric `time_t` value, or the special string `"never"`. + format, a numeric `time_t` value, or the special strings `"never"` or + `"now"`. See also **--show-last-upload-attempt-time**. diff --git a/util/stdlib/string_number_conversion.cc b/util/stdlib/string_number_conversion.cc index 28f6ea11..65d5a632 100644 --- a/util/stdlib/string_number_conversion.cc +++ b/util/stdlib/string_number_conversion.cc @@ -110,6 +110,13 @@ struct StringToUnsignedIntTraits } }; +struct StringToInt64Traits + : public StringToSignedIntegerTraits { + static LongType Convert(const char* str, char** end, int base) { + return strtoll(str, end, base); + } +}; + struct StringToUnsignedInt64Traits : public StringToUnsignedIntegerTraits { static LongType Convert(const char* str, char** end, int base) { @@ -166,6 +173,10 @@ bool StringToNumber(const base::StringPiece& string, unsigned int* number) { return StringToIntegerInternal(string, number); } +bool StringToNumber(const base::StringPiece& string, int64_t* number) { + return StringToIntegerInternal(string, number); +} + bool StringToNumber(const base::StringPiece& string, uint64_t* number) { return StringToIntegerInternal(string, number); } diff --git a/util/stdlib/string_number_conversion.h b/util/stdlib/string_number_conversion.h index 8641a690..b7bdcce8 100644 --- a/util/stdlib/string_number_conversion.h +++ b/util/stdlib/string_number_conversion.h @@ -56,6 +56,7 @@ namespace crashpad { //! where such prefix recognition is desirable. bool StringToNumber(const base::StringPiece& string, int* number); bool StringToNumber(const base::StringPiece& string, unsigned int* number); +bool StringToNumber(const base::StringPiece& string, int64_t* number); bool StringToNumber(const base::StringPiece& string, uint64_t* number); //! \} diff --git a/util/stdlib/string_number_conversion_test.cc b/util/stdlib/string_number_conversion_test.cc index 6fe6f813..318d9da6 100644 --- a/util/stdlib/string_number_conversion_test.cc +++ b/util/stdlib/string_number_conversion_test.cc @@ -221,6 +221,106 @@ TEST(StringNumberConversion, StringToUnsignedInt) { EXPECT_EQ(6u, output); } +TEST(StringNumberConversion, StringToInt64) { + const struct { + const char* string; + bool valid; + int64_t value; + } kTestData[] = { + {"", false, 0}, + {"0", true, 0}, + {"1", true, 1}, + {"2147483647", true, 2147483647}, + {"2147483648", true, 2147483648}, + {"4294967295", true, 4294967295}, + {"4294967296", true, 4294967296}, + {"9223372036854775807", true, std::numeric_limits::max()}, + {"9223372036854775808", false, 0}, + {"18446744073709551615", false, 0}, + {"18446744073709551616", false, 0}, + {"-1", true, -1}, + {"-2147483648", true, -2147483648}, + {"-2147483649", true, -2147483649}, + {"-2147483648", true, -2147483648}, + {"-9223372036854775808", true, std::numeric_limits::min()}, + {"-9223372036854775809", false, 0}, + {"0x7fffffffffffffff", true, std::numeric_limits::max()}, + {"0x8000000000000000", false, 0}, + {"0xffffffffffffffff", false, 0}, + {"0x10000000000000000", false, 0}, + {"-0x7fffffffffffffff", true, -9223372036854775807}, + {"-0x8000000000000000", true, std::numeric_limits::min()}, + {"-0x8000000000000001", false, 0}, + {"0x7Fffffffffffffff", true, std::numeric_limits::max()}, + }; + + for (size_t index = 0; index < arraysize(kTestData); ++index) { + int64_t value; + bool valid = StringToNumber(kTestData[index].string, &value); + if (kTestData[index].valid) { + EXPECT_TRUE(valid) << "index " << index << ", string " + << kTestData[index].string; + if (valid) { + EXPECT_EQ(kTestData[index].value, value) + << "index " << index << ", string " << kTestData[index].string; + } + } else { + EXPECT_FALSE(valid) << "index " << index << ", string " + << kTestData[index].string << ", value " << value; + } + } +} + +TEST(StringNumberConversion, StringToUnsignedInt64) { + const struct { + const char* string; + bool valid; + uint64_t value; + } kTestData[] = { + {"", false, 0}, + {"0", true, 0}, + {"1", true, 1}, + {"2147483647", true, 2147483647}, + {"2147483648", true, 2147483648}, + {"4294967295", true, 4294967295}, + {"4294967296", true, 4294967296}, + {"9223372036854775807", true, 9223372036854775807}, + {"9223372036854775808", true, 9223372036854775808u}, + {"18446744073709551615", true, std::numeric_limits::max()}, + {"18446744073709551616", false, 0}, + {"-1", false, 0}, + {"-2147483648", false, 0}, + {"-2147483649", false, 0}, + {"-2147483648", false, 0}, + {"-9223372036854775808", false, 0}, + {"-9223372036854775809", false, 0}, + {"0x7fffffffffffffff", true, 9223372036854775807}, + {"0x8000000000000000", true, 9223372036854775808u}, + {"0xffffffffffffffff", true, std::numeric_limits::max()}, + {"0x10000000000000000", false, 0}, + {"-0x7fffffffffffffff", false, 0}, + {"-0x8000000000000000", false, 0}, + {"-0x8000000000000001", false, 0}, + {"0xFfffffffffffffff", true, std::numeric_limits::max()}, + }; + + for (size_t index = 0; index < arraysize(kTestData); ++index) { + uint64_t value; + bool valid = StringToNumber(kTestData[index].string, &value); + if (kTestData[index].valid) { + EXPECT_TRUE(valid) << "index " << index << ", string " + << kTestData[index].string; + if (valid) { + EXPECT_EQ(kTestData[index].value, value) + << "index " << index << ", string " << kTestData[index].string; + } + } else { + EXPECT_FALSE(valid) << "index " << index << ", string " + << kTestData[index].string << ", value " << value; + } + } +} + } // namespace } // namespace test } // namespace crashpad From 5a21fc157380504b3dd7afc08621fe33efe39833 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 15 Nov 2016 14:12:23 -0500 Subject: [PATCH 04/19] Fix Windows build after f09d0cde00a1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While building crashpad_database_util.cc: …\crashpad\tools\crashpad_database_util.cc(150) : error C3861: 'gettimeofday': identifier not found util/win/time.h has its own GetTimeOfDay() to provide this missing function on Windows. I don’t know why it’s not in compat. Even so, it doesn’t return a value, so it’d be unsuitable for use in the PCHECK(). Go back to time() with an errno test. While building string_number_conversion_test.cc: …\crashpad\util\stdlib\string_number_conversion_test.cc(242) : error C2220: warning treated as error - no 'object' file generated …\crashpad\util\stdlib\string_number_conversion_test.cc(242) : warning C4146: unary minus operator applied to unsigned type, result still unsigned …\crashpad\util\stdlib\string_number_conversion_test.cc(243) : warning C4146: unary minus operator applied to unsigned type, result still unsigned …\crashpad\util\stdlib\string_number_conversion_test.cc(244) : warning C4146: unary minus operator applied to unsigned type, result still unsigned Use INT64_C(), and remove a duplicate test case. Change-Id: I308db9856e492604c7462238cb8b7b66731f0cfe Reviewed-on: https://chromium-review.googlesource.com/411331 Reviewed-by: Robert Sesek --- tools/crashpad_database_util.cc | 7 +++---- util/stdlib/string_number_conversion_test.cc | 5 ++--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/tools/crashpad_database_util.cc b/tools/crashpad_database_util.cc index a9aeb72b..e7d89bc4 100644 --- a/tools/crashpad_database_util.cc +++ b/tools/crashpad_database_util.cc @@ -12,12 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include #include #include -#include #include #include @@ -146,9 +146,8 @@ bool StringToTime(const char* string, time_t* out_time, bool utc) { } if (strcasecmp(string, "now") == 0) { - timeval tv; - PCHECK(gettimeofday(&tv, nullptr) == 0); - *out_time = tv.tv_sec; + errno = 0; + PCHECK(time(out_time) != -1 || errno == 0); return true; } diff --git a/util/stdlib/string_number_conversion_test.cc b/util/stdlib/string_number_conversion_test.cc index 318d9da6..c455a38d 100644 --- a/util/stdlib/string_number_conversion_test.cc +++ b/util/stdlib/string_number_conversion_test.cc @@ -239,9 +239,8 @@ TEST(StringNumberConversion, StringToInt64) { {"18446744073709551615", false, 0}, {"18446744073709551616", false, 0}, {"-1", true, -1}, - {"-2147483648", true, -2147483648}, - {"-2147483649", true, -2147483649}, - {"-2147483648", true, -2147483648}, + {"-2147483648", true, INT64_C(-2147483648)}, + {"-2147483649", true, INT64_C(-2147483649)}, {"-9223372036854775808", true, std::numeric_limits::min()}, {"-9223372036854775809", false, 0}, {"0x7fffffffffffffff", true, std::numeric_limits::max()}, From 8b3eec83e982af4138b038f077ddc62e8a061f20 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Thu, 17 Nov 2016 14:00:21 -0800 Subject: [PATCH 05/19] win: Add signal handler for SIGABRT to handle abort() calls R=mark@chromium.org BUG=crashpad:57 Change-Id: Ib7141f00e74e3db9e5be427cc990847331e09912 Reviewed-on: https://chromium-review.googlesource.com/412058 Reviewed-by: Mark Mentovai --- client/crashpad_client_win.cc | 55 +++++++++++++++++++- handler/handler.gyp | 14 +++++ handler/win/crashy_signal.cc | 91 +++++++++++++++++++++++++++++++++ snapshot/win/end_to_end_test.py | 24 +++++++++ 4 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 handler/win/crashy_signal.cc diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 235ef185..6f93f7ae 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -15,6 +15,7 @@ #include "client/crashpad_client.h" #include +#include #include #include @@ -31,6 +32,7 @@ #include "util/file/file_io.h" #include "util/misc/random_string.h" #include "util/win/address_types.h" +#include "util/win/capture_context.h" #include "util/win/command_line.h" #include "util/win/critical_section_with_debug_info.h" #include "util/win/get_function.h" @@ -163,6 +165,28 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { return EXCEPTION_CONTINUE_SEARCH; } +void HandleAbortSignal(int signum) { + DCHECK_EQ(signum, SIGABRT); + + CONTEXT context; + CaptureContext(&context); + + EXCEPTION_RECORD record = {}; + record.ExceptionCode = STATUS_FATAL_APP_EXIT; + record.ExceptionFlags = EXCEPTION_NONCONTINUABLE; +#if defined(ARCH_CPU_64_BITS) + record.ExceptionAddress = reinterpret_cast(context.Rip); +#else + record.ExceptionAddress = reinterpret_cast(context.Eip); +#endif // ARCH_CPU_64_BITS + + EXCEPTION_POINTERS exception_pointers; + exception_pointers.ContextRecord = &context; + exception_pointers.ExceptionRecord = &record; + + UnhandledExceptionHandler(&exception_pointers); +} + std::wstring FormatArgumentString(const std::string& name, const std::wstring& value) { return std::wstring(L"--") + base::UTF8ToUTF16(name) + L"=" + value; @@ -500,6 +524,32 @@ void CommonInProcessInitialization() { g_non_crash_dump_lock = new base::Lock(); } +void RegisterHandlers() { + SetUnhandledExceptionFilter(&UnhandledExceptionHandler); + + // The Windows CRT's signal.h lists: + // - SIGINT + // - SIGILL + // - SIGFPE + // - SIGSEGV + // - SIGTERM + // - SIGBREAK + // - SIGABRT + // SIGILL and SIGTERM are documented as not being generated. SIGBREAK and + // SIGINT are for Ctrl-Break and Ctrl-C, and aren't something for which + // capturing a dump is warranted. SIGFPE and SIGSEGV are captured as regular + // exceptions through the unhandled exception filter. This leaves SIGABRT. In + // the standard CRT, abort() is implemented as a synchronous call to the + // SIGABRT signal handler if installed, but after doing so, the unhandled + // exception filter is not triggered (it instead __fastfail()s). So, register + // to handle SIGABRT to catch abort() calls, as client code might use this and + // expect it to cause a crash dump. This will only work when the abort() + // that's called in client code is the same (or has the same behavior) as the + // one in use here. + _crt_signal_t rv = signal(SIGABRT, HandleAbortSignal); + DCHECK_NE(rv, SIG_ERR); +} + } // namespace CrashpadClient::CrashpadClient() : ipc_pipe_(), handler_start_thread_() {} @@ -536,7 +586,7 @@ bool CrashpadClient::StartHandler( CommonInProcessInitialization(); - SetUnhandledExceptionFilter(&UnhandledExceptionHandler); + RegisterHandlers(); auto data = new BackgroundHandlerStartThreadData(handler, database, @@ -609,7 +659,8 @@ bool CrashpadClient::SetHandlerIPCPipe(const std::wstring& ipc_pipe) { } SetHandlerStartupState(StartupState::kSucceeded); - SetUnhandledExceptionFilter(&UnhandledExceptionHandler); + + RegisterHandlers(); // The server returns these already duplicated to be valid in this process. g_signal_exception = diff --git a/handler/handler.gyp b/handler/handler.gyp index d6900531..e2d993df 100644 --- a/handler/handler.gyp +++ b/handler/handler.gyp @@ -147,6 +147,20 @@ 'win/crashy_test_program.cc', ], }, + { + 'target_name': 'crashy_signal', + 'type': 'executable', + 'dependencies': [ + '../client/client.gyp:crashpad_client', + '../third_party/mini_chromium/mini_chromium.gyp:base', + ], + 'include_dirs': [ + '..', + ], + 'sources': [ + 'win/crashy_signal.cc', + ], + }, { 'target_name': 'crash_other_program', 'type': 'executable', diff --git a/handler/win/crashy_signal.cc b/handler/win/crashy_signal.cc new file mode 100644 index 00000000..634bce8c --- /dev/null +++ b/handler/win/crashy_signal.cc @@ -0,0 +1,91 @@ +// Copyright 2016 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include + +#include "base/logging.h" +#include "base/strings/utf_string_conversions.h" +#include "client/crashpad_client.h" + +namespace crashpad { +namespace { + +enum WhereToSignalFrom { + kUnknown = -1, + kMain = 0, + kBackground = 1, +}; + +WhereToSignalFrom MainOrBackground(wchar_t* name) { + if (wcscmp(name, L"main") == 0) + return kMain; + if (wcscmp(name, L"background") == 0) + return kBackground; + return kUnknown; +} + +DWORD WINAPI BackgroundThread(void* arg) { + abort(); + return 0; +} + +int CrashySignalMain(int argc, wchar_t* argv[]) { + CrashpadClient client; + + WhereToSignalFrom from; + if (argc == 3 && (from = MainOrBackground(argv[2])) != kUnknown) { + if (!client.SetHandlerIPCPipe(argv[1])) { + LOG(ERROR) << "SetHandler"; + return EXIT_FAILURE; + } + } else { + LOG(ERROR) << "Usage: " << base::UTF16ToUTF8(argv[0]) + << " main|background"; + return EXIT_FAILURE; + } + + // In debug builds part of abort() is to open a dialog. We don't want tests to + // block at that dialog, so disable it. + _set_abort_behavior(0, _WRITE_ABORT_MSG); + + if (from == kBackground) { + HANDLE thread = CreateThread(nullptr, + 0, + &BackgroundThread, + nullptr, + 0, + nullptr); + if (!thread) { + PLOG(ERROR) << "CreateThread"; + return EXIT_FAILURE; + } + if (WaitForSingleObject(thread, INFINITE) != WAIT_OBJECT_0) { + PLOG(ERROR) << "WaitForSingleObject"; + return EXIT_FAILURE; + } + } else { + abort(); + } + + return EXIT_SUCCESS; +} + +} // namespace +} // namespace crashpad + +int wmain(int argc, wchar_t* argv[]) { + return crashpad::CrashySignalMain(argc, argv); +} diff --git a/snapshot/win/end_to_end_test.py b/snapshot/win/end_to_end_test.py index f469bcf4..42999df8 100755 --- a/snapshot/win/end_to_end_test.py +++ b/snapshot/win/end_to_end_test.py @@ -143,6 +143,10 @@ def GetDumpFromOtherProgram(out_dir, pipe_name, *args): *args) +def GetDumpFromSignal(out_dir, pipe_name, *args): + return GetDumpFromProgram(out_dir, pipe_name, 'crashy_signal.exe', *args) + + def GetDumpFromSelfDestroyingProgram(out_dir, pipe_name): return GetDumpFromProgram(out_dir, pipe_name, 'self_destroying_program.exe') @@ -201,6 +205,8 @@ def RunTests(cdb_path, z7_dump_path, other_program_path, other_program_no_exception_path, + sigabrt_main_path, + sigabrt_background_path, pipe_name): """Runs various tests in sequence. Runs a new cdb instance on the dump for each block of tests to reduce the chances that output from one command is @@ -361,6 +367,13 @@ def RunTests(cdb_path, 'other program with no exception given') out.Check('!RaiseException', 'other program in RaiseException()') + out = CdbRun(cdb_path, sigabrt_main_path, '.ecxr') + out.Check('code 40000015', 'got sigabrt signal') + out.Check('::HandleAbortSignal', ' stack in expected location') + + out = CdbRun(cdb_path, sigabrt_background_path, '.ecxr') + out.Check('code 40000015', 'got sigabrt signal from background thread') + def main(args): try: @@ -411,6 +424,15 @@ def main(args): if not other_program_no_exception_path: return 1 + sigabrt_main_path = GetDumpFromSignal(args[0], pipe_name, 'main') + if not sigabrt_main_path: + return 1 + + sigabrt_background_path = GetDumpFromSignal( + args[0], pipe_name, 'background') + if not sigabrt_background_path: + return 1 + RunTests(cdb_path, crashy_dump_path, start_handler_dump_path, @@ -418,6 +440,8 @@ def main(args): z7_dump_path, other_program_path, other_program_no_exception_path, + sigabrt_main_path, + sigabrt_background_path, pipe_name) return 1 if g_had_failures else 0 From 68095b6a4e6148125fe631c63469496d2afca29c Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Thu, 17 Nov 2016 14:18:06 -0800 Subject: [PATCH 06/19] Don't LOG(ERROR) for usage, and fix VS2013 build after 8b3eec8 R=mark@chromium.org BUG=crashpad:57 Change-Id: I6514a82ae5de38a695422ef86c044ec3b2ce171b Reviewed-on: https://chromium-review.googlesource.com/412269 Reviewed-by: Mark Mentovai --- client/crashpad_client_win.cc | 2 +- handler/win/crashy_signal.cc | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 6f93f7ae..0eee1857 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -546,7 +546,7 @@ void RegisterHandlers() { // expect it to cause a crash dump. This will only work when the abort() // that's called in client code is the same (or has the same behavior) as the // one in use here. - _crt_signal_t rv = signal(SIGABRT, HandleAbortSignal); + void (*rv)(int) = signal(SIGABRT, HandleAbortSignal); DCHECK_NE(rv, SIG_ERR); } diff --git a/handler/win/crashy_signal.cc b/handler/win/crashy_signal.cc index 634bce8c..8caa6259 100644 --- a/handler/win/crashy_signal.cc +++ b/handler/win/crashy_signal.cc @@ -12,12 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include #include "base/logging.h" -#include "base/strings/utf_string_conversions.h" #include "client/crashpad_client.h" namespace crashpad { @@ -52,8 +52,7 @@ int CrashySignalMain(int argc, wchar_t* argv[]) { return EXIT_FAILURE; } } else { - LOG(ERROR) << "Usage: " << base::UTF16ToUTF8(argv[0]) - << " main|background"; + fprintf(stderr, "Usage: %ls main|background\n", argv[0]); return EXIT_FAILURE; } From 777634b1ebad7149d0af41686f1f36126b4ea984 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 22 Nov 2016 14:27:51 -0500 Subject: [PATCH 07/19] Use ADDRESS_SANITIZER instead of __has_feature(address_sanitizer) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit __has_feature() is a Clang-ism not implemented by GCC. base/compiler_specific.h provides a HAS_FEATURE() macro that always returns 0 when __has_feature() is not implemented. Use this macro for compatibility with GCC and other compilers that do not implement this Clang extension. http://clang.llvm.org/docs/LanguageExtensions.html#has-feature-and-has-extension For GCC’s Address Sanitizer implementation, test the __SANITIZE_ADDRESS__ macro that it provides as an alternative to __has_feature(address_sanitizer). Note that in Chrome builds, ADDRESS_SANITIZER is pushed in by the build system. The definition of ADDRESS_SANITIZER provides another way for that macro to be set. It’s supplementary, not exclusive. https://chromium.googlesource.com/chromium/src/+/cb33b243721b0e7dbdfa8c4301702e319360e88d/build/config/BUILD.gn#118 BUG=crashpad:30 Change-Id: I5c3145d29bbc966925369c03a37b1ecb5622a004 Reviewed-on: https://chromium-review.googlesource.com/413109 Reviewed-by: Robert Sesek --- client/capture_context_mac_test.cc | 6 ++++-- client/crashpad_info.cc | 5 +++-- util/misc/address_sanitizer.h | 28 ++++++++++++++++++++++++++++ util/util.gyp | 1 + 4 files changed, 36 insertions(+), 4 deletions(-) create mode 100644 util/misc/address_sanitizer.h diff --git a/client/capture_context_mac_test.cc b/client/capture_context_mac_test.cc index 436ac5ad..1904b2b6 100644 --- a/client/capture_context_mac_test.cc +++ b/client/capture_context_mac_test.cc @@ -21,6 +21,7 @@ #include "build/build_config.h" #include "gtest/gtest.h" +#include "util/misc/address_sanitizer.h" #include "util/misc/implicit_cast.h" namespace crashpad { @@ -103,13 +104,14 @@ void TestCaptureContext() { // captured program counter should be slightly greater than or equal to the // reference program counter. uintptr_t pc = ProgramCounterFromContext(context_1); -#if !__has_feature(address_sanitizer) + +#if !defined(ADDRESS_SANITIZER) // AddressSanitizer can cause enough code bloat that the “nearby” check would // likely fail. const uintptr_t kReferencePC = reinterpret_cast(TestCaptureContext); EXPECT_LT(pc - kReferencePC, 64u); -#endif +#endif // !defined(ADDRESS_SANITIZER) // Declare sp and context_2 here because all local variables need to be // declared before computing the stack pointer reference value, so that the diff --git a/client/crashpad_info.cc b/client/crashpad_info.cc index e8a6a9ee..e517f7b1 100644 --- a/client/crashpad_info.cc +++ b/client/crashpad_info.cc @@ -14,6 +14,7 @@ #include "client/crashpad_info.h" +#include "util/misc/address_sanitizer.h" #include "util/stdlib/cxx.h" #if defined(OS_MACOSX) @@ -72,14 +73,14 @@ __attribute__(( #error Port #endif // !defined(OS_MACOSX) && !defined(OS_LINUX) && !defined(OS_ANDROID) -#if __has_feature(address_sanitizer) +#if defined(ADDRESS_SANITIZER) // AddressSanitizer would add a trailing red zone of at least 32 bytes, // which would be reflected in the size of the custom section. This confuses // MachOImageReader::GetCrashpadInfo(), which finds that the section’s size // disagrees with the structure’s size_ field. By specifying an alignment // greater than the red zone size, the red zone will be suppressed. aligned(64), -#endif // __has_feature(address_sanitizer) +#endif // defined(ADDRESS_SANITIZER) // The “used” attribute prevents the structure from being dead-stripped. used, diff --git a/util/misc/address_sanitizer.h b/util/misc/address_sanitizer.h new file mode 100644 index 00000000..75c72399 --- /dev/null +++ b/util/misc/address_sanitizer.h @@ -0,0 +1,28 @@ +// Copyright 2016 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_UTIL_MISC_ADDRESS_SANITIZER_H_ +#define CRASHPAD_UTIL_MISC_ADDRESS_SANITIZER_H_ + +#include "base/compiler_specific.h" +#include "build/build_config.h" + +#if !defined(ADDRESS_SANITIZER) +#if HAS_FEATURE(address_sanitizer) || \ + (defined(COMPILER_GCC) && defined(__SANITIZE_ADDRESS__)) +#define ADDRESS_SANITIZER 1 +#endif +#endif // !defined(ADDRESS_SANITIZER) + +#endif // CRASHPAD_UTIL_MISC_ADDRESS_SANITIZER_H_ diff --git a/util/util.gyp b/util/util.gyp index 5768788a..418256d3 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -84,6 +84,7 @@ 'mach/task_for_pid.h', 'mach/task_memory.cc', 'mach/task_memory.h', + 'misc/address_sanitizer.h', 'misc/arraysize_unsafe.h', 'misc/clock.h', 'misc/clock_mac.cc', From 5b83e58771e25f7e7c8c84e854defa8f464457a2 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 7 Dec 2016 11:35:07 -0800 Subject: [PATCH 08/19] win: Remove use of rpcrt4 and advapi32 from some util code ConvertStringSecurityDescriptorToSecurityDescriptor() is used when creating the initial connection pipe. Because this is done from inside DllMain(), we cannot use advapi32 (where this function is). Instead, save the binary representation of the self-relative SECURITY_DESCRIPTOR. It is conceivable that this could change, but unlikely as this is the same blob that would be stored on a file in NTFS. Another potential approach would be to not make the pipe available to all integrity levels here, and instead modify the Chromium sandbox code to allow a specific pipe name prefix that would have to correspond with the pipe name that Crashpad creates. Similarly, UuidCreate() (used when initializing the database) is in a DLL that can't be loaded early, so use the Linux/Android implementation on Windows too. R=mark@chromium.org BUG=chromium:655788,chromium:656800 Change-Id: I434f8e96fc275fc30d0a31208b025bfc08595ff9 Reviewed-on: https://chromium-review.googlesource.com/417223 Reviewed-by: Mark Mentovai --- util/misc/uuid.cc | 12 +-- util/util.gyp | 2 - util/util_test.gyp | 2 + util/win/registration_protocol_win.cc | 101 +++++++++++++++++---- util/win/registration_protocol_win.h | 12 +++ util/win/registration_protocol_win_test.cc | 53 +++++++++++ 6 files changed, 155 insertions(+), 27 deletions(-) create mode 100644 util/win/registration_protocol_win_test.cc diff --git a/util/misc/uuid.cc b/util/misc/uuid.cc index 4f1fbcbe..d4345a8a 100644 --- a/util/misc/uuid.cc +++ b/util/misc/uuid.cc @@ -95,17 +95,11 @@ bool UUID::InitializeWithNew() { uuid_generate(uuid); InitializeFromBytes(uuid); return true; -#elif defined(OS_WIN) - ::UUID system_uuid; - if (UuidCreate(&system_uuid) != RPC_S_OK) { - LOG(ERROR) << "UuidCreate"; - return false; - } - InitializeFromSystemUUID(&system_uuid); - return true; -#elif defined(OS_LINUX) || defined(OS_ANDROID) +#elif defined(OS_WIN) || defined(OS_LINUX) || defined(OS_ANDROID) // Linux does not provide a UUID generator in a widely-available system // library. uuid_generate() from libuuid is not available everywhere. + // On Windows, do not use UuidCreate() to avoid a dependency on rpcrt4, so + // that this function is usable early in DllMain(). base::RandBytes(this, sizeof(*this)); // Set six bits per RFC 4122 §4.4 to identify this as a pseudo-random UUID. diff --git a/util/util.gyp b/util/util.gyp index 418256d3..abf0bfdc 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -263,8 +263,6 @@ ['OS=="win"', { 'link_settings': { 'libraries': [ - '-ladvapi32.lib', - '-lrpcrt4.lib', '-lwinhttp.lib', ], }, diff --git a/util/util_test.gyp b/util/util_test.gyp index b5989de9..e6bf5635 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -91,6 +91,7 @@ 'win/handle_test.cc', 'win/initial_client_data_test.cc', 'win/process_info_test.cc', + 'win/registration_protocol_win_test.cc', 'win/scoped_process_suspend_test.cc', 'win/time_test.cc', ], @@ -108,6 +109,7 @@ ], 'link_settings': { 'libraries': [ + '-ladvapi32.lib', '-limagehlp.lib', '-lrpcrt4.lib', ], diff --git a/util/win/registration_protocol_win.cc b/util/win/registration_protocol_win.cc index 17638415..ebadd60f 100644 --- a/util/win/registration_protocol_win.cc +++ b/util/win/registration_protocol_win.cc @@ -15,12 +15,11 @@ #include "util/win/registration_protocol_win.h" #include -#include #include "base/logging.h" +#include "base/macros.h" #include "util/win/exception_handler_server.h" #include "util/win/scoped_handle.h" -#include "util/win/scoped_local_alloc.h" namespace crashpad { @@ -97,7 +96,6 @@ HANDLE CreateNamedPipeInstance(const std::wstring& pipe_name, bool first_instance) { SECURITY_ATTRIBUTES security_attributes; SECURITY_ATTRIBUTES* security_attributes_pointer = nullptr; - ScopedLocalAlloc scoped_sec_desc; if (first_instance) { // Pre-Vista does not have integrity levels. @@ -105,21 +103,10 @@ HANDLE CreateNamedPipeInstance(const std::wstring& pipe_name, const DWORD major_version = LOBYTE(LOWORD(version)); const bool is_vista_or_later = major_version >= 6; if (is_vista_or_later) { - // Mandatory Label, no ACE flags, no ObjectType, integrity level - // untrusted. - const wchar_t kSddl[] = L"S:(ML;;;;;S-1-16-0)"; - - PSECURITY_DESCRIPTOR sec_desc; - PCHECK(ConvertStringSecurityDescriptorToSecurityDescriptor( - kSddl, SDDL_REVISION_1, &sec_desc, nullptr)) - << "ConvertStringSecurityDescriptorToSecurityDescriptor"; - - // Take ownership of the allocated SECURITY_DESCRIPTOR. - scoped_sec_desc.reset(sec_desc); - memset(&security_attributes, 0, sizeof(security_attributes)); security_attributes.nLength = sizeof(SECURITY_ATTRIBUTES); - security_attributes.lpSecurityDescriptor = sec_desc; + security_attributes.lpSecurityDescriptor = + const_cast(GetSecurityDescriptorForNamedPipeInstance(nullptr)); security_attributes.bInheritHandle = TRUE; security_attributes_pointer = &security_attributes; } @@ -136,4 +123,86 @@ HANDLE CreateNamedPipeInstance(const std::wstring& pipe_name, security_attributes_pointer); } +const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size) { + // Mandatory Label, no ACE flags, no ObjectType, integrity level untrusted is + // "S:(ML;;;;;S-1-16-0)". Typically + // ConvertStringSecurityDescriptorToSecurityDescriptor() would be used to + // convert from a string representation. However, that function cannot be used + // because it is in advapi32.dll and CreateNamedPipeInstance() is called from + // within DllMain() where the loader lock is held. advapi32.dll is delay + // loaded in chrome_elf.dll because it must avoid loading user32.dll. If an + // advapi32.dll function were used, it would cause a load of the DLL, which + // would in turn cause deadlock. + +#pragma pack(push, 1) + static const struct SecurityDescriptorBlob { + // See https://msdn.microsoft.com/en-us/library/cc230366.aspx. + SECURITY_DESCRIPTOR_RELATIVE sd_rel; + struct { + ACL acl; + struct { + // This is equivalent to SYSTEM_MANDATORY_LABEL_ACE, but there's no + // DWORD offset to the SID, instead it's inline. + ACE_HEADER header; + ACCESS_MASK mask; + SID sid; + } ace[1]; + } sacl; + } kSecDescBlob = { + // sd_rel. + { + SECURITY_DESCRIPTOR_REVISION1, // Revision. + 0x00, // Sbz1. + SE_SELF_RELATIVE | SE_SACL_PRESENT, // Control. + 0, // OffsetOwner. + 0, // OffsetGroup. + offsetof(SecurityDescriptorBlob, sacl), // OffsetSacl. + 0, // OffsetDacl. + }, + + // sacl. + { + // acl. + { + ACL_REVISION, // AclRevision. + 0, // Sbz1. + sizeof(SecurityDescriptorBlob::sacl), // AclSize. + arraysize(SecurityDescriptorBlob::sacl.ace), // AceCount. + 0, // Sbz2. + }, + + // ace[0]. + { + { + // header. + { + SYSTEM_MANDATORY_LABEL_ACE_TYPE, // AceType. + 0, // AceFlags. + sizeof(SecurityDescriptorBlob::sacl.ace[0]), // AceSize. + }, + + // mask. + 0, + + // sid. + { + SID_REVISION, // Revision. + // SubAuthorityCount. + arraysize( + SecurityDescriptorBlob::sacl.ace[0].sid.SubAuthority), + // IdentifierAuthority. + SECURITY_MANDATORY_LABEL_AUTHORITY, + {SECURITY_MANDATORY_UNTRUSTED_RID}, // SubAuthority. + }, + }, + }, + }, + }; +#pragma pack(pop) + + if (size) + *size = sizeof(kSecDescBlob); + return reinterpret_cast(&kSecDescBlob); +} + } // namespace crashpad diff --git a/util/win/registration_protocol_win.h b/util/win/registration_protocol_win.h index 12209835..5f04a466 100644 --- a/util/win/registration_protocol_win.h +++ b/util/win/registration_protocol_win.h @@ -145,6 +145,18 @@ bool SendToCrashHandlerServer(const base::string16& pipe_name, HANDLE CreateNamedPipeInstance(const std::wstring& pipe_name, bool first_instance); +//! \brief Returns the SECURITY_DESCRIPTOR blob that will be used for creating +//! the connection pipe in CreateNamedPipeInstance(). +//! +//! This function is exposed for only for testing. +//! +//! \param[out] size The size of the returned blob. May be `nullptr` if not +//! required. +//! +//! \return A pointer to a self-relative `SECURITY_DESCRIPTOR`. Ownership is not +//! transferred to the caller. +const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size); + } // namespace crashpad #endif // CRASHPAD_UTIL_WIN_REGISTRATION_PROTOCOL_WIN_H_ diff --git a/util/win/registration_protocol_win_test.cc b/util/win/registration_protocol_win_test.cc new file mode 100644 index 00000000..60e7c86c --- /dev/null +++ b/util/win/registration_protocol_win_test.cc @@ -0,0 +1,53 @@ +// Copyright 2016 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/win/registration_protocol_win.h" + +#include +#include +#include + +#include "gtest/gtest.h" +#include "test/errors.h" +#include "util/win/scoped_local_alloc.h" + +namespace crashpad { +namespace test { +namespace { + +TEST(SecurityDescriptor, MatchesAdvapi32) { + // This security descriptor is built manually in the connection code to avoid + // calling the advapi32 functions. Verify that it returns the same thing as + // ConvertStringSecurityDescriptorToSecurityDescriptor() would. + + // Mandatory Label, no ACE flags, no ObjectType, integrity level + // untrusted. + const wchar_t kSddl[] = L"S:(ML;;;;;S-1-16-0)"; + PSECURITY_DESCRIPTOR sec_desc; + ULONG sec_desc_len; + ASSERT_TRUE(ConvertStringSecurityDescriptorToSecurityDescriptor( + kSddl, SDDL_REVISION_1, &sec_desc, &sec_desc_len)) + << ErrorMessage("ConvertStringSecurityDescriptorToSecurityDescriptor"); + ScopedLocalAlloc sec_desc_owner(sec_desc); + + size_t created_len; + const void* const created = + GetSecurityDescriptorForNamedPipeInstance(&created_len); + ASSERT_EQ(sec_desc_len, created_len); + EXPECT_EQ(0, memcmp(sec_desc, created, sec_desc_len)); +} + +} // namespace +} // namespace test +} // namespace crashpad From f94dd14c45ead99d84ec84bc4460ff470d83fc38 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 7 Dec 2016 13:25:02 -0800 Subject: [PATCH 09/19] win: fix SECURITY_DESCRIPTOR builder on vs2013 After https://chromium.googlesource.com/crashpad/crashpad/+/5b83e587. R=mark@chromium.org BUG=chromium:655788,chromium:656800 Change-Id: Ic33b9daedc340bfce3cc4ddde4eb4c93f68e7ad0 Reviewed-on: https://chromium-review.googlesource.com/417412 Reviewed-by: Mark Mentovai --- util/win/registration_protocol_win.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/util/win/registration_protocol_win.cc b/util/win/registration_protocol_win.cc index ebadd60f..b270d24c 100644 --- a/util/win/registration_protocol_win.cc +++ b/util/win/registration_protocol_win.cc @@ -166,8 +166,8 @@ const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size) { { ACL_REVISION, // AclRevision. 0, // Sbz1. - sizeof(SecurityDescriptorBlob::sacl), // AclSize. - arraysize(SecurityDescriptorBlob::sacl.ace), // AceCount. + sizeof(kSecDescBlob.sacl), // AclSize. + arraysize(kSecDescBlob.sacl.ace), // AceCount. 0, // Sbz2. }, @@ -178,7 +178,7 @@ const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size) { { SYSTEM_MANDATORY_LABEL_ACE_TYPE, // AceType. 0, // AceFlags. - sizeof(SecurityDescriptorBlob::sacl.ace[0]), // AceSize. + sizeof(kSecDescBlob.sacl.ace[0]), // AceSize. }, // mask. @@ -188,8 +188,7 @@ const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size) { { SID_REVISION, // Revision. // SubAuthorityCount. - arraysize( - SecurityDescriptorBlob::sacl.ace[0].sid.SubAuthority), + arraysize(kSecDescBlob.sacl.ace[0].sid.SubAuthority), // IdentifierAuthority. SECURITY_MANDATORY_LABEL_AUTHORITY, {SECURITY_MANDATORY_UNTRUSTED_RID}, // SubAuthority. From 556c4e4f50453bd4e7146b20ba8b408fe5f8c319 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 7 Dec 2016 13:42:50 -0800 Subject: [PATCH 10/19] Have crashpad call ASan's crash handler if present Upstreaming change made downstream in https://codereview.chromium.org/2504773002. Formatting modified slightly. R=mark@chromium.org, rnk@chromium.org BUG=661209 Change-Id: Iab8c4ffda3af24b7a61ec0a4a10b187966da481f Reviewed-on: https://chromium-review.googlesource.com/417237 Reviewed-by: Mark Mentovai --- client/crashpad_client_win.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 0eee1857..becfc019 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -109,7 +109,18 @@ StartupState BlockUntilHandlerStartedOrFailed() { return static_cast(startup_state); } +#if defined(ADDRESS_SANITIZER) +extern "C" LONG __asan_unhandled_exception_filter(EXCEPTION_POINTERS* info); +#endif + LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { +#if defined(ADDRESS_SANITIZER) + // In ASan builds, delegate to the ASan exception filter. + LONG status = __asan_unhandled_exception_filter(exception_pointers); + if (status != EXCEPTION_CONTINUE_SEARCH) + return status; +#endif + if (BlockUntilHandlerStartedOrFailed() == StartupState::kFailed) { // If we know for certain that the handler has failed to start, then abort // here, rather than trying to signal to a handler that will never arrive, From f66d5df30cb85a637c197bbab1541367f92f3c62 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 7 Dec 2016 14:41:30 -0800 Subject: [PATCH 11/19] Roll mini_chromium to de1afb0 > git log 414d596..de1afb0 --oneline de1afb0 Update base/numerics from Chromium ca7f42a Improve the Win32/x64 configuration when generating MSVS projects c1f7a2c Create initial GN configuration for mini_chromium. R=mark@chromium.org Change-Id: I309fe722c18764c9a85e9c6e212f39bf07fe3b02 Reviewed-on: https://chromium-review.googlesource.com/417770 Reviewed-by: Mark Mentovai --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index 7a9be6d3..8615c43c 100644 --- a/DEPS +++ b/DEPS @@ -25,7 +25,7 @@ deps = { '93cc6e2c23e4d5ebd179f388e67aa907d0dfd43d', 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - '414d59602ac38e24f1e93929fda3d79d72cea139', + 'de1afb04f4afc074ec6d23bd9ee7b1e6b365427f', 'buildtools': Var('chromium_git') + '/chromium/buildtools.git@' + 'f8fc76ea5ce4a60cda2fa5d7df3d4a62935b3113', From 6b09b08a22028c4e699f4afdc6c00eef7de8e3fe Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 7 Dec 2016 14:53:16 -0800 Subject: [PATCH 12/19] Update util/file/string_file.cc for new base/numerics API The code was not incorrect before, but this expression is simpler. Upstream of change made at https://codereview.chromium.org/2528243002. R=mark@chromium.org BUG=chromium:668713 Change-Id: Idae36bd8312666a3254eda02713869776fec0248 Reviewed-on: https://chromium-review.googlesource.com/417981 Reviewed-by: Mark Mentovai --- util/file/string_file.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/util/file/string_file.cc b/util/file/string_file.cc index 1c74b3f0..959b0a53 100644 --- a/util/file/string_file.cc +++ b/util/file/string_file.cc @@ -157,17 +157,16 @@ FileOffset StringFile::Seek(FileOffset offset, int whence) { LOG(ERROR) << "Seek(): new_offset invalid"; return -1; } - FileOffset new_offset_fileoffset = new_offset.ValueOrDie(); size_t new_offset_sizet; - if (!AssignIfInRange(&new_offset_sizet, new_offset_fileoffset)) { - LOG(ERROR) << "Seek(): new_offset " << new_offset_fileoffset + if (!new_offset.AssignIfValid(&new_offset_sizet)) { + LOG(ERROR) << "Seek(): new_offset " << new_offset.ValueOrDie() << " invalid for size_t"; return -1; } offset_ = new_offset_sizet; - return offset_.ValueOrDie(); + return base::ValueOrDieForType(offset_); } } // namespace crashpad From 32981a3ee9d7c2769fb27afa038fe2e194cfa329 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Thu, 8 Dec 2016 09:58:46 -0800 Subject: [PATCH 13/19] win: Fix clang warning in SECURITY_DESCRIPTOR construction c:\src\cr\src\third_party\crashpad\crashpad\util\win\registration_protocol_win.cc(193,23): error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] SECURITY_MANDATORY_LABEL_AUTHORITY, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ c:\src\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\win_sdk\Include\10.0.10586.0\um\winnt.h(9068,54): note: expanded from macro 'SECURITY_MANDATORY_LABEL_AUTHORITY' ^~~~~~~~~~~~ 1 error generated. R=mark@chromium.org BUG=chromium:656800 Change-Id: I1121a42ca98d8a7432e247d4b44a9ad1214d4b39 Reviewed-on: https://chromium-review.googlesource.com/418010 Reviewed-by: Mark Mentovai --- util/win/registration_protocol_win.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/win/registration_protocol_win.cc b/util/win/registration_protocol_win.cc index b270d24c..e5cbd8d0 100644 --- a/util/win/registration_protocol_win.cc +++ b/util/win/registration_protocol_win.cc @@ -190,7 +190,7 @@ const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size) { // SubAuthorityCount. arraysize(kSecDescBlob.sacl.ace[0].sid.SubAuthority), // IdentifierAuthority. - SECURITY_MANDATORY_LABEL_AUTHORITY, + {SECURITY_MANDATORY_LABEL_AUTHORITY}, {SECURITY_MANDATORY_UNTRUSTED_RID}, // SubAuthority. }, }, From cdbb90ec6968f369a93a8071576f44208eddabea Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Mon, 12 Dec 2016 20:57:48 -0800 Subject: [PATCH 14/19] win: Add timeout argument to WaitForHandlerStart() As brought up in https://codereview.chromium.org/2475863004/, there's the potential for failed startup if StartHandlerProcess() hangs for whatever reason. Add a timeout to the wait function so that this case can attempt to log an error. R=mark@chromium.org BUG=655788, 656800, 565063 Change-Id: Ib08cd0641daa6a6cefabb773ffe470227b51958c Reviewed-on: https://chromium-review.googlesource.com/419060 Reviewed-by: Mark Mentovai --- client/crashpad_client.h | 5 ++++- client/crashpad_client_win.cc | 12 +++++++++--- client/crashpad_client_win_test.cc | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/client/crashpad_client.h b/client/crashpad_client.h index 844512f1..f6f3395c 100644 --- a/client/crashpad_client.h +++ b/client/crashpad_client.h @@ -173,9 +173,12 @@ class CrashpadClient { //! //! This method should not be used unless `asynchronous_start` was `true`. //! + //! \param[in] timeout_ms The number of milliseconds to wait for a result from + //! the background launch, or `0xffffffff` to block indefinitely. + //! //! \return `true` if the hander startup succeeded, `false` otherwise, and an //! error message will have been logged. - bool WaitForHandlerStart(); + bool WaitForHandlerStart(unsigned int timeout_ms); //! \brief Requests that the handler capture a dump even though there hasn't //! been a crash. diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index becfc019..7a3d0047 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -689,10 +689,16 @@ std::wstring CrashpadClient::GetHandlerIPCPipe() const { return ipc_pipe_; } -bool CrashpadClient::WaitForHandlerStart() { +bool CrashpadClient::WaitForHandlerStart(unsigned int timeout_ms) { DCHECK(handler_start_thread_.is_valid()); - if (WaitForSingleObject(handler_start_thread_.get(), INFINITE) != - WAIT_OBJECT_0) { + DWORD result = WaitForSingleObject(handler_start_thread_.get(), timeout_ms); + if (result == WAIT_TIMEOUT) { + LOG(ERROR) << "WaitForSingleObject timed out"; + return false; + } else if (result == WAIT_ABANDONED) { + LOG(ERROR) << "WaitForSingleObject abandoned"; + return false; + } else if (result != WAIT_OBJECT_0) { PLOG(ERROR) << "WaitForSingleObject"; return false; } diff --git a/client/crashpad_client_win_test.cc b/client/crashpad_client_win_test.cc index 94a9b0cc..47e7add9 100644 --- a/client/crashpad_client_win_test.cc +++ b/client/crashpad_client_win_test.cc @@ -39,7 +39,7 @@ void StartAndUseHandler() { std::vector(), true, true)); - ASSERT_TRUE(client.WaitForHandlerStart()); + ASSERT_TRUE(client.WaitForHandlerStart(INFINITE)); } class StartWithInvalidHandles final : public WinMultiprocess { From 2e80cb7cb484e26c26fc45fc7ea5e4082218f9e1 Mon Sep 17 00:00:00 2001 From: Sigurdur Asgeirsson Date: Thu, 15 Dec 2016 14:50:58 -0500 Subject: [PATCH 15/19] win: Delegate to previous UEF on exception Change-Id: I02f6d048d8a51797f93794ecc761f4fc8ba139a7 Reviewed-on: https://chromium-review.googlesource.com/420849 Reviewed-by: Mark Mentovai --- handler/handler_main.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/handler/handler_main.cc b/handler/handler_main.cc index 29c5ddc1..3ada8c3e 100644 --- a/handler/handler_main.cc +++ b/handler/handler_main.cc @@ -129,9 +129,15 @@ void HandleSIGTERM(int sig, siginfo_t* siginfo, void* context) { #endif // OS_MACOSX #if defined(OS_WIN) +LONG(WINAPI* g_original_exception_filter)(EXCEPTION_POINTERS*) = nullptr; + LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { Metrics::HandlerCrashed(exception_pointers->ExceptionRecord->ExceptionCode); - return EXCEPTION_CONTINUE_SEARCH; + + if (g_original_exception_filter) + return g_original_exception_filter(exception_pointers); + else + return EXCEPTION_CONTINUE_SEARCH; } #endif // OS_WIN @@ -139,7 +145,8 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { int HandlerMain(int argc, char* argv[]) { #if defined(OS_WIN) - SetUnhandledExceptionFilter(&UnhandledExceptionHandler); + g_original_exception_filter = + SetUnhandledExceptionFilter(&UnhandledExceptionHandler); #endif const base::FilePath argv0( From 0567536f86fb10f9663fb30d6ebf08a7c35b975d Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Fri, 16 Dec 2016 10:04:18 -0800 Subject: [PATCH 16/19] win: Attempt to fix unloaded modules list by using RtlGetUnloadEventTraceEx I haven't been able to reproduce this locally, but we see errors in crash dumps where the unloaded module list consists of a number of modules with invalid names and implausible addresses. My assumption is that RTL_UNLOAD_EVENT_TRACE isn't correct for some OS levels. Instead of trying to finesse and test that, use RtlGetUnloadEventTraceEx() instead of RtlGetUnloadEventTrace(), which returns an element size. (This function is Vista+ which is why it wasn't used the first time around.) R=mark@chromium.org BUG=chromium:620175 Change-Id: I4d7080a03623276f9c1c038d6e7329af70e4a64c Reviewed-on: https://chromium-review.googlesource.com/421564 Reviewed-by: Mark Mentovai --- handler/win/crashy_test_program.cc | 4 +-- snapshot/win/process_snapshot_win.cc | 44 +++++++++++++++++++++------- util/win/nt_internals.cc | 22 ++++++-------- util/win/nt_internals.h | 15 +++------- 4 files changed, 48 insertions(+), 37 deletions(-) diff --git a/handler/win/crashy_test_program.cc b/handler/win/crashy_test_program.cc index b6e044c8..58afa47f 100644 --- a/handler/win/crashy_test_program.cc +++ b/handler/win/crashy_test_program.cc @@ -201,8 +201,8 @@ int CrashyMain(int argc, wchar_t* argv[]) { AllocateExtraUnsavedMemory(extra_ranges); // Load and unload some uncommonly used modules so we can see them in the list - // reported by `lm`. At least two so that we confirm we got the size of - // RTL_UNLOAD_EVENT_TRACE right. + // reported by `lm`. At least two so that we confirm we got the element size + // advancement of RTL_UNLOAD_EVENT_TRACE correct. CHECK(GetModuleHandle(L"lz32.dll") == nullptr); CHECK(GetModuleHandle(L"wmerror.dll") == nullptr); HMODULE lz32 = LoadLibrary(L"lz32.dll"); diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc index 63543c76..11e83145 100644 --- a/snapshot/win/process_snapshot_win.cc +++ b/snapshot/win/process_snapshot_win.cc @@ -281,21 +281,43 @@ void ProcessSnapshotWin::InitializeUnloadedModules() { #error port #endif - RTL_UNLOAD_EVENT_TRACE* unload_event_trace_address = - RtlGetUnloadEventTrace(); - WinVMAddress address_in_target_process = - reinterpret_cast(unload_event_trace_address); + ULONG* element_size; + ULONG* element_count; + void* event_trace_address; + RtlGetUnloadEventTraceEx(&element_size, &element_count, &event_trace_address); - std::vector> events( - RTL_UNLOAD_EVENT_TRACE_NUMBER); - if (!process_reader_.ReadMemory(address_in_target_process, - events.size() * sizeof(events[0]), - &events[0])) { + if (*element_size < sizeof(RTL_UNLOAD_EVENT_TRACE)) { + LOG(ERROR) << "unexpected unloaded module list element size"; return; } - for (const RTL_UNLOAD_EVENT_TRACE& uet : events) { - if (uet.ImageName[0]) { + const WinVMAddress address_in_target_process = + reinterpret_cast(event_trace_address); + + Traits::Pointer pointer_to_array; + if (!process_reader_.ReadMemory(address_in_target_process, + sizeof(pointer_to_array), + &pointer_to_array)) { + LOG(ERROR) << "failed to read target address"; + return; + } + + // No unloaded modules. + if (pointer_to_array == 0) + return; + + const size_t data_size = *element_size * *element_count; + std::vector data(data_size); + if (!process_reader_.ReadMemory(pointer_to_array, data_size, &data[0])) { + LOG(ERROR) << "failed to read unloaded module data"; + return; + } + + for (ULONG i = 0; i < *element_count; ++i) { + const uint8_t* base_address = &data[i * *element_size]; + const auto& uet = + *reinterpret_cast*>(base_address); + if (uet.ImageName[0] != 0) { unloaded_modules_.push_back(UnloadedModuleSnapshot( uet.BaseAddress, uet.SizeOfImage, diff --git a/util/win/nt_internals.cc b/util/win/nt_internals.cc index 12201620..97eba68b 100644 --- a/util/win/nt_internals.cc +++ b/util/win/nt_internals.cc @@ -42,7 +42,9 @@ NTSTATUS NTAPI NtSuspendProcess(HANDLE); NTSTATUS NTAPI NtResumeProcess(HANDLE); -void* NTAPI RtlGetUnloadEventTrace(); +VOID NTAPI RtlGetUnloadEventTraceEx(PULONG* ElementSize, + PULONG* ElementCount, + PVOID* EventTrace); namespace crashpad { @@ -145,12 +147,12 @@ NTSTATUS NtResumeProcess(HANDLE handle) { return nt_resume_process(handle); } -template -RTL_UNLOAD_EVENT_TRACE* RtlGetUnloadEventTrace() { - static const auto rtl_get_unload_event_trace = - GET_FUNCTION_REQUIRED(L"ntdll.dll", ::RtlGetUnloadEventTrace); - return reinterpret_cast*>( - rtl_get_unload_event_trace()); +void RtlGetUnloadEventTraceEx(ULONG** element_size, + ULONG** element_count, + void** event_trace) { + static const auto rtl_get_unload_event_trace_ex = + GET_FUNCTION_REQUIRED(L"ntdll.dll", ::RtlGetUnloadEventTraceEx); + rtl_get_unload_event_trace_ex(element_size, element_count, event_trace); } // Explicit instantiations with the only 2 valid template arguments to avoid @@ -169,10 +171,4 @@ template NTSTATUS NtOpenThread( const process_types::CLIENT_ID* client_id); -template RTL_UNLOAD_EVENT_TRACE* -RtlGetUnloadEventTrace(); - -template RTL_UNLOAD_EVENT_TRACE* -RtlGetUnloadEventTrace(); - } // namespace crashpad diff --git a/util/win/nt_internals.h b/util/win/nt_internals.h index 0a80fd53..41e5fa44 100644 --- a/util/win/nt_internals.h +++ b/util/win/nt_internals.h @@ -75,10 +75,7 @@ NTSTATUS NtSuspendProcess(HANDLE handle); NTSTATUS NtResumeProcess(HANDLE handle); -// From https://msdn.microsoft.com/en-us/library/bb432428(VS.85).aspx and -// http://processhacker.sourceforge.net/doc/struct___r_t_l___u_n_l_o_a_d___e_v_e_n_t___t_r_a_c_e.html -#define RTL_UNLOAD_EVENT_TRACE_NUMBER 64 - +// From https://msdn.microsoft.com/en-us/library/cc678403(v=vs.85).aspx. template struct RTL_UNLOAD_EVENT_TRACE { typename Traits::Pointer BaseAddress; @@ -87,14 +84,10 @@ struct RTL_UNLOAD_EVENT_TRACE { ULONG TimeDateStamp; ULONG CheckSum; WCHAR ImageName[32]; - ULONG Version0; - union { - ULONG Version1; - typename Traits::Pad alignment_for_x64; - }; }; -template -RTL_UNLOAD_EVENT_TRACE* RtlGetUnloadEventTrace(); +void RtlGetUnloadEventTraceEx(ULONG** element_size, + ULONG** element_count, + void** event_trace); } // namespace crashpad From 2d68949f7ff7502c3c6fd988552308abec495c9f Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 4 Jan 2017 18:56:45 -0800 Subject: [PATCH 17/19] Pull LLVM/Clang/LLDB into third_party This might feel a bit premature, but I feel fairly confident that basing a lot of the processing on LLDB is the way to go, so I plan to start by integrating it into our build process. (I think probably moving from gyp to GN first makes sense, so I can defer landing this until after that's farther along if you like.) BUG=crashpad:29 Change-Id: I85ee44f4e777f9d7ce521c4caf10ead21ffd8818 Reviewed-on: https://chromium-review.googlesource.com/424910 Reviewed-by: Mark Mentovai --- .gitignore | 1 + DEPS | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index c4f8fbb3..65bb4418 100644 --- a/.gitignore +++ b/.gitignore @@ -12,6 +12,7 @@ /out /third_party/gtest/gtest /third_party/gyp/gyp +/third_party/llvm /third_party/mini_chromium/mini_chromium /xcodebuild tags diff --git a/DEPS b/DEPS index 8615c43c..520a7624 100644 --- a/DEPS +++ b/DEPS @@ -17,18 +17,28 @@ vars = { } deps = { + 'buildtools': + Var('chromium_git') + '/chromium/buildtools.git@' + + 'f8fc76ea5ce4a60cda2fa5d7df3d4a62935b3113', 'crashpad/third_party/gtest/gtest': Var('chromium_git') + '/external/github.com/google/googletest@' + 'ec44c6c1675c25b9827aacd08c02433cccde7780', 'crashpad/third_party/gyp/gyp': Var('chromium_git') + '/external/gyp@' + '93cc6e2c23e4d5ebd179f388e67aa907d0dfd43d', + + # TODO(scottmg): Consider pinning these. For now, we don't have any particular + # reason to do so. + 'crashpad/third_party/llvm': + Var('chromium_git') + '/external/llvm.org/llvm.git@HEAD', + 'crashpad/third_party/llvm/tools/clang': + Var('chromium_git') + '/external/llvm.org/clang.git@HEAD', + 'crashpad/third_party/llvm/tools/lldb': + Var('chromium_git') + '/external/llvm.org/lldb.git@HEAD', + 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + 'de1afb04f4afc074ec6d23bd9ee7b1e6b365427f', - 'buildtools': - Var('chromium_git') + '/chromium/buildtools.git@' + - 'f8fc76ea5ce4a60cda2fa5d7df3d4a62935b3113', } hooks = [ From f9b3a18f3f44a775689f42d5aec77fe97bc79108 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Thu, 5 Jan 2017 13:44:55 -0800 Subject: [PATCH 18/19] Use DEPS hooks to get gn binaries from buildtools Otherwise: [2d68949...]D:\src\crashpad\crashpad>gn gen out\Debug gn.py: Could not find gn executable at: D:\src\crashpad\buildtools\win\gn.exe I have no idea why these binaries aren't just checked into buildtools, but anyway. BUG=crashpad:79 Change-Id: If2f21a7e7f795910809de7d3595ab6a5ffee9dc7 Reviewed-on: https://chromium-review.googlesource.com/424847 Reviewed-by: Mark Mentovai --- DEPS | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/DEPS b/DEPS index 520a7624..77428536 100644 --- a/DEPS +++ b/DEPS @@ -51,7 +51,6 @@ hooks = [ '--no_resume', '--no_auth', '--bucket=chromium-clang-format', - '--output=buildtools/mac/clang-format', '--sha1_file', 'buildtools/mac/clang-format.sha1', ], @@ -65,11 +64,36 @@ hooks = [ '--no_resume', '--no_auth', '--bucket=chromium-clang-format', - '--output=buildtools/win/clang-format.exe', '--sha1_file', 'buildtools/win/clang-format.exe.sha1', ], }, + { + 'name': 'gn_mac', + 'pattern': '.', + 'action': [ + 'download_from_google_storage', + '--platform=^darwin$', + '--no_resume', + '--no_auth', + '--bucket=chromium-gn', + '--sha1_file', + 'buildtools/mac/gn.sha1', + ], + }, + { + 'name': 'gn_win', + 'pattern': '.', + 'action': [ + 'download_from_google_storage', + '--platform=^win32$', + '--no_resume', + '--no_auth', + '--bucket=chromium-gn', + '--sha1_file', + 'buildtools/win/gn.exe.sha1', + ], + }, { 'name': 'gyp', 'pattern': '\.gypi?$', From e7630628e9c93084fdd47d478542d81928c50e03 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 24 Jan 2017 10:59:18 -0500 Subject: [PATCH 19/19] mac: Report richer exception codes via metrics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, only the top-level exception code was reported via the Crashpad.ExceptionCode.Mac histogram. Making this histogram work (https://crbug.com/678720) has revealed that Chrome is triggering EXC_RESOURCE exceptions at a rate in excess of 4x that of ordinary crashes. These exceptions were not previously visible because they are not uploaded unless the system treats them as fatal, which it does not normally do absent an explicit request. In order to learn more about the problem, this change augments the data reported via the Crashpad.ExceptionCode.Mac histogram to report (at least) second-level exception data. This means that we will no longer see just EXC_RESOURCE, but potentially more useful information such as EXC_RESOURCE / RESOURCE_TYPE_IO / FLAVOR_IO_PHYSICAL_WRITES. This also applies to other exception types, so that the majority of crashes currently falling into the EXC_CRASH bucket will now have additional information decoded and will be reported as, for example, EXC_BAD_ACCESS / KERN_INVALID_ADDRESS, EXC_BAD_INSTRUCTION / EXC_I386_INVOP, and EXC_CRASH / SIGABRT. Because the old mechanism was only live (in an “it works” sense) for several days, and the new mechanism does not overlap with histogram values used by the old one, there’s no need to invent a new histogram name. BUG=chromium:684051 Change-Id: Ia0a372b4127f7b3b2e7dbbaac9304cce3b5aadfe Reviewed-on: https://chromium-review.googlesource.com/430933 Reviewed-by: Scott Graham --- handler/mac/crash_report_exception_handler.cc | 2 +- snapshot/mac/exception_snapshot_mac.cc | 15 +- util/mach/exception_types.cc | 110 +++++++++++ util/mach/exception_types.h | 50 +++++ util/mach/exception_types_test.cc | 187 ++++++++++++++++-- 5 files changed, 328 insertions(+), 36 deletions(-) diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc index 2a115623..7784afef 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -66,7 +66,7 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( const mach_msg_trailer_t* trailer, bool* destroy_complex_request) { Metrics::ExceptionEncountered(); - Metrics::ExceptionCode(exception); + Metrics::ExceptionCode(ExceptionCodeForMetrics(exception, code[0])); *destroy_complex_request = true; // The expected behavior is EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES, diff --git a/snapshot/mac/exception_snapshot_mac.cc b/snapshot/mac/exception_snapshot_mac.cc index c5d32b5b..92c8450b 100644 --- a/snapshot/mac/exception_snapshot_mac.cc +++ b/snapshot/mac/exception_snapshot_mac.cc @@ -66,20 +66,7 @@ bool ExceptionSnapshotMac::Initialize(ProcessReader* process_reader, exception_ = ExcCrashRecoverOriginalException( exception_code_0, &exception_code_0, nullptr); - if (exception_ == EXC_CRASH || - exception_ == EXC_RESOURCE || - exception_ == EXC_GUARD) { - // EXC_CRASH should never be wrapped in another EXC_CRASH. - // - // EXC_RESOURCE and EXC_GUARD are software exceptions that are never - // wrapped in EXC_CRASH. The only time EXC_CRASH is generated is for - // processes exiting due to an unhandled core-generating signal or being - // killed by SIGKILL for code-signing reasons. Neither of these applies to - // EXC_RESOURCE or EXC_GUARD. See 10.10 xnu-2782.1.97/bsd/kern/kern_exit.c - // proc_prepareexit(). Receiving these exception types wrapped in - // EXC_CRASH would lose information because their code[0] uses all 64 bits - // (see below) and the code[0] recovered from EXC_CRASH only contains 20 - // significant bits. + if (!ExcCrashCouldContainException(exception_)) { LOG(WARNING) << base::StringPrintf( "exception %s invalid in EXC_CRASH", ExceptionToString(exception_, kUseFullName | kUnknownIsNumeric) diff --git a/util/mach/exception_types.cc b/util/mach/exception_types.cc index 09366b57..85bdf770 100644 --- a/util/mach/exception_types.cc +++ b/util/mach/exception_types.cc @@ -24,6 +24,8 @@ #include "base/logging.h" #include "base/mac/mach_logging.h" #include "util/mac/mac_util.h" +#include "util/mach/mach_extensions.h" +#include "util/numeric/in_range_cast.h" #if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_9 @@ -128,6 +130,114 @@ exception_type_t ExcCrashRecoverOriginalException( return (code_0 >> 20) & 0xf; } +bool ExcCrashCouldContainException(exception_type_t exception) { + // EXC_CRASH should never be wrapped in another EXC_CRASH. + // + // EXC_RESOURCE and EXC_GUARD are software exceptions that are never wrapped + // in EXC_CRASH. The only time EXC_CRASH is generated is for processes exiting + // due to an unhandled core-generating signal or being killed by SIGKILL for + // code-signing reasons. Neither of these apply to EXC_RESOURCE or EXC_GUARD. + // See 10.10 xnu-2782.1.97/bsd/kern/kern_exit.c proc_prepareexit(). Receiving + // these exception types wrapped in EXC_CRASH would lose information because + // their code[0] uses all 64 bits (see ExceptionSnapshotMac::Initialize()) and + // the code[0] recovered from EXC_CRASH only contains 20 significant bits. + // + // EXC_CORPSE_NOTIFY may be generated from EXC_CRASH, but the opposite should + // never occur. + // + // kMachExceptionSimulated is a non-fatal Crashpad-specific pseudo-exception + // that never exists as an exception within the kernel and should thus never + // be wrapped in EXC_CRASH. + return exception != EXC_CRASH && + exception != EXC_RESOURCE && + exception != EXC_GUARD && + exception != EXC_CORPSE_NOTIFY && + exception != kMachExceptionSimulated; +} + +int32_t ExceptionCodeForMetrics(exception_type_t exception, + mach_exception_code_t code_0) { + if (exception == kMachExceptionSimulated) { + return exception; + } + + int signal = 0; + if (exception == EXC_CRASH) { + const exception_type_t original_exception = + ExcCrashRecoverOriginalException(code_0, &code_0, &signal); + if (!ExcCrashCouldContainException(original_exception)) { + LOG(WARNING) << "EXC_CRASH should not contain exception " + << original_exception; + return InRangeCast(original_exception, 0xffff) << 16; + } + exception = original_exception; + } + + uint16_t metrics_exception = InRangeCast(exception, 0xffff); + + uint16_t metrics_code_0; + switch (exception) { + case EXC_RESOURCE: + metrics_code_0 = (EXC_RESOURCE_DECODE_RESOURCE_TYPE(code_0) << 8) | + EXC_RESOURCE_DECODE_FLAVOR(code_0); + break; + + case EXC_GUARD: { + // This will be GUARD_TYPE_MACH_PORT (1) from or + // GUARD_TYPE_FD (2) from 10.12.2 xnu-3789.31.2/bsd/sys/guarded.h + const uint8_t guard_type = (code_0) >> 61; + + // These exceptions come through 10.12.2 + // xnu-3789.31.2/osfmk/ipc/mach_port.c mach_port_guard_exception() or + // xnu-3789.31.2/bsd/kern/kern_guarded.c fp_guard_exception(). In each + // case, bits 32-60 of code_0 encode the guard type-specific “flavor”. For + // Mach port guards, these flavor codes come from the + // mach_port_guard_exception_codes enum in . For file + // descriptor guards, they come from the guard_exception_codes enum in + // xnu-3789.31.2/bsd/sys/guarded.h. Both of these enums define shifted-bit + // values (1 << 0, 1 << 1, 1 << 2, etc.) In actual usage as determined by + // callers to these functions, these “flavor” codes are never ORed with + // 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); + + metrics_guard_flavor = bit; + break; + } + } + + metrics_code_0 = (guard_type << 8) | metrics_guard_flavor; + break; + } + + case EXC_CORPSE_NOTIFY: + // code_0 may be a pointer. See 10.12.2 xnu-3789.31.2/osfmk/kern/task.c + // task_deliver_crash_notification(). Just encode 0 for metrics purposes. + metrics_code_0 = 0; + break; + + default: + metrics_code_0 = InRangeCast(code_0, 0xffff); + if (exception == 0 && metrics_code_0 == 0 && signal != 0) { + // This exception came from a signal that did not originate as another + // Mach exception. Encode the signal number, using EXC_CRASH as the + // top-level exception type. This is safe because EXC_CRASH will not + // otherwise appear as metrics_exception. + metrics_exception = EXC_CRASH; + metrics_code_0 = signal; + } + break; + } + + return (metrics_exception << 16) | metrics_code_0; +} + bool IsExceptionNonfatalResource(exception_type_t exception, mach_exception_code_t code_0, pid_t pid) { diff --git a/util/mach/exception_types.h b/util/mach/exception_types.h index 834f2123..5082ed19 100644 --- a/util/mach/exception_types.h +++ b/util/mach/exception_types.h @@ -15,6 +15,7 @@ #ifndef CRASHPAD_UTIL_MACH_EXCEPTION_TYPES_H_ #define CRASHPAD_UTIL_MACH_EXCEPTION_TYPES_H_ +#include #include #include @@ -54,6 +55,55 @@ exception_type_t ExcCrashRecoverOriginalException( mach_exception_code_t* original_code_0, int* signal); +//! \brief Determines whether a given exception type could plausibly be carried +//! within an `EXC_CRASH` exception. +//! +//! \param[in] exception The exception type to test. +//! +//! \return `true` if an `EXC_CRASH` exception could plausibly carry \a +//! exception. +//! +//! An `EXC_CRASH` exception can wrap exceptions that originate as hardware +//! faults, as well as exceptions that originate from certain software sources +//! such as POSIX signals. It cannot wrap another `EXC_CRASH` exception, nor can +//! it wrap `EXC_RESOURCE`, `EXC_GUARD`, or `EXC_CORPSE_NOTIFY` exceptions. It +//! also cannot wrap Crashpad-specific #kMachExceptionSimulated exceptions. +bool ExcCrashCouldContainException(exception_type_t exception); + +//! \brief Returns the exception code to report via a configured metrics system. +//! +//! \param[in] exception The exception type as received by a Mach exception +//! handler. +//! \param[in] code_0 The first exception code (`code[0]`) as received by a +//! Mach exception handler. +//! +//! \return An exception code that maps useful information from \a exception and +//! \a code_0 to the more limited data type available for metrics reporting. +//! +//! For classic Mach exceptions (including hardware faults reported as Mach +//! exceptions), the mapping is `(exception << 16) | code_0`. +//! +//! For `EXC_CRASH` exceptions that originate as Mach exceptions described +//! above, the mapping above is used, with the original exception’s values. For +//! `EXC_CRASH` exceptions that originate as POSIX signals without an underlying +//! Mach exception, the mapping is `(EXC_CRASH << 16) | code_0`. +//! +//! `EXC_RESOURCE` and `EXC_GUARD` exceptions both contain exception-specific +//! “type” values and type-specific “flavor” values. In these cases, the mapping +//! is `(exception << 16) | (type << 8) | flavor`. For `EXC_GUARD`, the “flavor” +//! value is rewritten to be more space-efficient by replacing the +//! kernel-supplied bitmask having exactly one bit set with the index of the set +//! bit. +//! +//! `EXC_CORPSE_NOTIFY` exceptions are reported as classic Mach exceptions with +//! the \a code_0 field set to `0`. +//! +//! If \a exception is #kMachExceptionSimulated, that value is returned as-is. +//! +//! Overflow conditions in any field are handled via saturation. +int32_t ExceptionCodeForMetrics(exception_type_t exception, + mach_exception_code_t code_0); + //! \brief Determines whether an exception is a non-fatal `EXC_RESOURCE`. //! //! \param[in] exception The exception type as received by a Mach exception diff --git a/util/mach/exception_types_test.cc b/util/mach/exception_types_test.cc index d82dcbb9..7ecdd252 100644 --- a/util/mach/exception_types_test.cc +++ b/util/mach/exception_types_test.cc @@ -41,9 +41,21 @@ TEST(ExceptionTypes, ExcCrashRecoverOriginalException) { {0xb100001, EXC_BAD_ACCESS, KERN_INVALID_ADDRESS, SIGSEGV}, {0xb100002, EXC_BAD_ACCESS, KERN_PROTECTION_FAILURE, SIGSEGV}, {0xa100002, EXC_BAD_ACCESS, KERN_PROTECTION_FAILURE, SIGBUS}, - {0x4200001, EXC_BAD_INSTRUCTION, 1, SIGILL}, - {0x8300001, EXC_ARITHMETIC, 1, SIGFPE}, - {0x5600002, EXC_BREAKPOINT, 2, SIGTRAP}, + {0xa100005, EXC_BAD_ACCESS, VM_PROT_READ | VM_PROT_EXECUTE, SIGBUS}, + {0xa10000d, EXC_BAD_ACCESS, EXC_I386_GPFLT, SIGBUS}, + {0x9100032, EXC_BAD_ACCESS, KERN_CODESIGN_ERROR, SIGKILL}, + {0x4200001, EXC_BAD_INSTRUCTION, EXC_I386_INVOP, SIGILL}, + {0x420000b, EXC_BAD_INSTRUCTION, EXC_I386_SEGNPFLT, SIGILL}, + {0x420000c, EXC_BAD_INSTRUCTION, EXC_I386_STKFLT, SIGILL}, + {0x8300001, EXC_ARITHMETIC, EXC_I386_DIV, SIGFPE}, + {0x8300002, EXC_ARITHMETIC, EXC_I386_INTO, SIGFPE}, + {0x8300005, EXC_ARITHMETIC, EXC_I386_EXTERR, SIGFPE}, + {0x8300008, EXC_ARITHMETIC, EXC_I386_SSEEXTERR, SIGFPE}, + {0x5500007, EXC_SOFTWARE, EXC_I386_BOUND, SIGTRAP}, + {0x5600001, EXC_BREAKPOINT, EXC_I386_SGL, SIGTRAP}, + {0x5600002, EXC_BREAKPOINT, EXC_I386_BPT, SIGTRAP}, + {0x0700080, EXC_SYSCALL, 128, 0}, + {0x0706000, EXC_SYSCALL, 0x6000, 0}, {0x3000000, 0, 0, SIGQUIT}, {0x6000000, 0, 0, SIGABRT}, {0xc000000, 0, 0, SIGSYS}, @@ -86,40 +98,173 @@ TEST(ExceptionTypes, ExcCrashRecoverOriginalException) { EXPECT_EQ(test_data.signal, signal); } -// These macros come from the #ifdef KERNEL section of : -// 10.10 xnu-2782.1.97/osfmk/kern/exc_resource.h. -#define EXC_RESOURCE_ENCODE_TYPE(code, type) \ - ((code) |= ((static_cast(type) & 0x7ull) << 61)) -#define EXC_RESOURCE_ENCODE_FLAVOR(code, flavor) \ - ((code) |= ((static_cast(flavor) & 0x7ull) << 58)) +TEST(ExceptionTypes, ExcCrashCouldContainException) { + // This seems wrong, but it happens when EXC_CRASH carries an exception not + // originally caused by a hardware fault, such as SIGABRT. + EXPECT_TRUE(ExcCrashCouldContainException(0)); + + EXPECT_TRUE(ExcCrashCouldContainException(EXC_BAD_ACCESS)); + EXPECT_TRUE(ExcCrashCouldContainException(EXC_BAD_INSTRUCTION)); + EXPECT_TRUE(ExcCrashCouldContainException(EXC_ARITHMETIC)); + EXPECT_TRUE(ExcCrashCouldContainException(EXC_EMULATION)); + EXPECT_TRUE(ExcCrashCouldContainException(EXC_SOFTWARE)); + EXPECT_TRUE(ExcCrashCouldContainException(EXC_BREAKPOINT)); + EXPECT_TRUE(ExcCrashCouldContainException(EXC_SYSCALL)); + EXPECT_TRUE(ExcCrashCouldContainException(EXC_MACH_SYSCALL)); + EXPECT_TRUE(ExcCrashCouldContainException(EXC_RPC_ALERT)); + EXPECT_FALSE(ExcCrashCouldContainException(EXC_CRASH)); + EXPECT_FALSE(ExcCrashCouldContainException(EXC_RESOURCE)); + EXPECT_FALSE(ExcCrashCouldContainException(EXC_GUARD)); + EXPECT_FALSE(ExcCrashCouldContainException(EXC_CORPSE_NOTIFY)); + EXPECT_FALSE(ExcCrashCouldContainException(kMachExceptionSimulated)); +} + +// This macro is adapted from those in the #ifdef KERNEL section of +// : 10.12.2 xnu-3789.31.2/osfmk/kern/exc_resource.h. +#define EXC_RESOURCE_ENCODE_TYPE_FLAVOR(type, flavor) \ + (static_cast( \ + (((static_cast(type) & 0x7ull) << 61) | \ + (static_cast(flavor) & 0x7ull) << 58))) + +TEST(ExceptionTypes, ExceptionCodeForMetrics) { + struct TestData { + exception_type_t exception; + mach_exception_code_t code_0; + int32_t metrics_code; + }; + const TestData kTestData[] = { +#define ENCODE_EXC(type, code_0) \ + { (type), (code_0), ((type) << 16) | (code_0) } + ENCODE_EXC(EXC_BAD_ACCESS, KERN_INVALID_ADDRESS), + ENCODE_EXC(EXC_BAD_ACCESS, KERN_PROTECTION_FAILURE), + ENCODE_EXC(EXC_BAD_ACCESS, VM_PROT_READ | VM_PROT_EXECUTE), + ENCODE_EXC(EXC_BAD_ACCESS, EXC_I386_GPFLT), + ENCODE_EXC(EXC_BAD_ACCESS, KERN_CODESIGN_ERROR), + ENCODE_EXC(EXC_BAD_INSTRUCTION, EXC_I386_INVOP), + ENCODE_EXC(EXC_BAD_INSTRUCTION, EXC_I386_SEGNPFLT), + ENCODE_EXC(EXC_BAD_INSTRUCTION, EXC_I386_STKFLT), + ENCODE_EXC(EXC_ARITHMETIC, EXC_I386_DIV), + ENCODE_EXC(EXC_ARITHMETIC, EXC_I386_INTO), + ENCODE_EXC(EXC_ARITHMETIC, EXC_I386_EXTERR), + ENCODE_EXC(EXC_ARITHMETIC, EXC_I386_SSEEXTERR), + ENCODE_EXC(EXC_SOFTWARE, EXC_I386_BOUND), + ENCODE_EXC(EXC_BREAKPOINT, EXC_I386_SGL), + ENCODE_EXC(EXC_BREAKPOINT, EXC_I386_BPT), + ENCODE_EXC(EXC_SYSCALL, 128), + ENCODE_EXC(EXC_SYSCALL, 0x6000), +#undef ENCODE_EXC + +#define ENCODE_EXC_CRASH(type, code_0) \ + { \ + EXC_CRASH, (((type) & 0xf) << 20) | ((code_0) & 0xfffff), \ + ((type) << 16) | (code_0) \ + } + ENCODE_EXC_CRASH(EXC_BAD_ACCESS, KERN_INVALID_ADDRESS), + ENCODE_EXC_CRASH(EXC_BAD_ACCESS, KERN_PROTECTION_FAILURE), + ENCODE_EXC_CRASH(EXC_BAD_ACCESS, VM_PROT_READ | VM_PROT_EXECUTE), + ENCODE_EXC_CRASH(EXC_BAD_ACCESS, EXC_I386_GPFLT), + ENCODE_EXC_CRASH(EXC_BAD_ACCESS, KERN_CODESIGN_ERROR), + ENCODE_EXC_CRASH(EXC_BAD_INSTRUCTION, EXC_I386_INVOP), + ENCODE_EXC_CRASH(EXC_BAD_INSTRUCTION, EXC_I386_SEGNPFLT), + ENCODE_EXC_CRASH(EXC_BAD_INSTRUCTION, EXC_I386_STKFLT), + ENCODE_EXC_CRASH(EXC_ARITHMETIC, EXC_I386_DIV), + ENCODE_EXC_CRASH(EXC_ARITHMETIC, EXC_I386_INTO), + ENCODE_EXC_CRASH(EXC_ARITHMETIC, EXC_I386_EXTERR), + ENCODE_EXC_CRASH(EXC_ARITHMETIC, EXC_I386_SSEEXTERR), + ENCODE_EXC_CRASH(EXC_SOFTWARE, EXC_I386_BOUND), + ENCODE_EXC_CRASH(EXC_BREAKPOINT, EXC_I386_SGL), + ENCODE_EXC_CRASH(EXC_BREAKPOINT, EXC_I386_BPT), + ENCODE_EXC_CRASH(EXC_SYSCALL, 128), + ENCODE_EXC_CRASH(EXC_SYSCALL, 0x6000), +#undef ENCODE_EXC_CRASH + +#define ENCODE_EXC_CRASH_SIGNAL(signal) \ + { EXC_CRASH, (((signal) & 0xff) << 24), (EXC_CRASH << 16) | (signal) } + ENCODE_EXC_CRASH_SIGNAL(SIGQUIT), + ENCODE_EXC_CRASH_SIGNAL(SIGABRT), + ENCODE_EXC_CRASH_SIGNAL(SIGSYS), +#undef ENCODE_EXC_CRASH_SIGNAL + +#define ENCODE_EXC_RESOURCE(type, flavor) \ + { \ + EXC_RESOURCE, EXC_RESOURCE_ENCODE_TYPE_FLAVOR((type), (flavor)), \ + (EXC_RESOURCE << 16) | ((type) << 8) | (flavor) \ + } + ENCODE_EXC_RESOURCE(RESOURCE_TYPE_CPU, FLAVOR_CPU_MONITOR), + ENCODE_EXC_RESOURCE(RESOURCE_TYPE_CPU, FLAVOR_CPU_MONITOR_FATAL), + ENCODE_EXC_RESOURCE(RESOURCE_TYPE_WAKEUPS, FLAVOR_WAKEUPS_MONITOR), + ENCODE_EXC_RESOURCE(RESOURCE_TYPE_MEMORY, FLAVOR_HIGH_WATERMARK), + ENCODE_EXC_RESOURCE(RESOURCE_TYPE_IO, FLAVOR_IO_PHYSICAL_WRITES), + ENCODE_EXC_RESOURCE(RESOURCE_TYPE_IO, FLAVOR_IO_LOGICAL_WRITES), +#undef ENCODE_EXC_RESOURCE + +#define ENCODE_EXC_GUARD(type, flavor) \ + { \ + EXC_GUARD, \ + static_cast(static_cast((type) & 0x7) \ + << 61) | \ + (static_cast((1 << (flavor)) & 0x1ffffffff) << 32), \ + (EXC_GUARD << 16) | ((type) << 8) | (flavor) \ + } + ENCODE_EXC_GUARD(GUARD_TYPE_MACH_PORT, 0), // kGUARD_EXC_DESTROY + ENCODE_EXC_GUARD(GUARD_TYPE_MACH_PORT, 1), // kGUARD_EXC_MOD_REFS + ENCODE_EXC_GUARD(GUARD_TYPE_MACH_PORT, 2), // kGUARD_EXC_SET_CONTEXT + ENCODE_EXC_GUARD(GUARD_TYPE_MACH_PORT, 3), // kGUARD_EXC_UNGUARDED + ENCODE_EXC_GUARD(GUARD_TYPE_MACH_PORT, 4), // kGUARD_EXC_INCORRECT_GUARD + + // 2 is GUARD_TYPE_FD from 10.12.2 xnu-3789.31.2/bsd/sys/guarded.h. + ENCODE_EXC_GUARD(2, 0), // kGUARD_EXC_CLOSE + ENCODE_EXC_GUARD(2, 1), // kGUARD_EXC_DUP + ENCODE_EXC_GUARD(2, 2), // kGUARD_EXC_NOCLOEXEC + ENCODE_EXC_GUARD(2, 3), // kGUARD_EXC_SOCKET_IPC + ENCODE_EXC_GUARD(2, 4), // kGUARD_EXC_FILEPORT + ENCODE_EXC_GUARD(2, 5), // kGUARD_EXC_MISMATCH + ENCODE_EXC_GUARD(2, 6), // kGUARD_EXC_WRITE +#undef ENCODE_EXC_GUARD + + // Test that overflow saturates. + {0x00010000, 0x00001000, static_cast(0xffff1000)}, + {0x00001000, 0x00010000, 0x1000ffff}, + {0x00010000, 0x00010000, static_cast(0xffffffff)}, + }; + + for (size_t index = 0; index < arraysize(kTestData); ++index) { + const TestData& test_data = kTestData[index]; + SCOPED_TRACE(base::StringPrintf("index %zu, exception 0x%x, code_0 0x%llx", + index, + test_data.exception, + test_data.code_0)); + + int32_t metrics_code = + ExceptionCodeForMetrics(test_data.exception, test_data.code_0); + + EXPECT_EQ(test_data.metrics_code, metrics_code); + } +} TEST(ExceptionTypes, IsExceptionNonfatalResource) { const pid_t pid = getpid(); - mach_exception_code_t code = 0; - EXC_RESOURCE_ENCODE_TYPE(code, RESOURCE_TYPE_CPU); - EXC_RESOURCE_ENCODE_FLAVOR(code, FLAVOR_CPU_MONITOR); + mach_exception_code_t code = + EXC_RESOURCE_ENCODE_TYPE_FLAVOR(RESOURCE_TYPE_CPU, FLAVOR_CPU_MONITOR); EXPECT_TRUE(IsExceptionNonfatalResource(EXC_RESOURCE, code, pid)); if (MacOSXMinorVersion() >= 10) { // FLAVOR_CPU_MONITOR_FATAL was introduced in OS X 10.10. - code = 0; - EXC_RESOURCE_ENCODE_TYPE(code, RESOURCE_TYPE_CPU); - EXC_RESOURCE_ENCODE_FLAVOR(code, FLAVOR_CPU_MONITOR_FATAL); + code = EXC_RESOURCE_ENCODE_TYPE_FLAVOR(RESOURCE_TYPE_CPU, + FLAVOR_CPU_MONITOR_FATAL); EXPECT_FALSE(IsExceptionNonfatalResource(EXC_RESOURCE, code, pid)); } // This assumes that WAKEMON_MAKE_FATAL is not set for this process. The // default is for WAKEMON_MAKE_FATAL to not be set, there’s no public API to // enable it, and nothing in this process should have enabled it. - code = 0; - EXC_RESOURCE_ENCODE_TYPE(code, RESOURCE_TYPE_WAKEUPS); - EXC_RESOURCE_ENCODE_FLAVOR(code, FLAVOR_WAKEUPS_MONITOR); + code = EXC_RESOURCE_ENCODE_TYPE_FLAVOR(RESOURCE_TYPE_WAKEUPS, + FLAVOR_WAKEUPS_MONITOR); EXPECT_TRUE(IsExceptionNonfatalResource(EXC_RESOURCE, code, pid)); - code = 0; - EXC_RESOURCE_ENCODE_TYPE(code, RESOURCE_TYPE_MEMORY); - EXC_RESOURCE_ENCODE_FLAVOR(code, FLAVOR_HIGH_WATERMARK); + code = EXC_RESOURCE_ENCODE_TYPE_FLAVOR(RESOURCE_TYPE_MEMORY, + FLAVOR_HIGH_WATERMARK); EXPECT_TRUE(IsExceptionNonfatalResource(EXC_RESOURCE, code, pid)); // Non-EXC_RESOURCE exceptions should never be considered nonfatal resource