From 23b2156fb694fcc8b2da03fa9e1d02cf81f53fe9 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 2 Mar 2018 21:45:29 -0500 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20read=20beyond=20a=20StringPiece?= =?UTF-8?q?=E2=80=99s=20bounds=20in=20StringToNumber()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The implementations requires NUL-termination for the underlying buffer, so just use std::string everywhere, rather than trying to detect whether strings are already NUL-terminated. Bug: chromium:817982, chromium:818376 Change-Id: I4c8dcb5ed15ebca4c531f9a5d0ee865228dc0959 Reviewed-on: https://chromium-review.googlesource.com/947742 Reviewed-by: Mark Mentovai Commit-Queue: Mark Mentovai --- util/linux/memory_map.cc | 2 +- util/mach/symbolic_constants_mach.cc | 11 +++++++---- util/posix/symbolic_constants_posix.cc | 2 +- util/stdlib/string_number_conversion.cc | 18 +++++------------- util/stdlib/string_number_conversion.h | 10 +++++----- util/stdlib/string_number_conversion_test.cc | 12 ++---------- 6 files changed, 21 insertions(+), 34 deletions(-) diff --git a/util/linux/memory_map.cc b/util/linux/memory_map.cc index cb7bfe1e..f2df865a 100644 --- a/util/linux/memory_map.cc +++ b/util/linux/memory_map.cc @@ -36,7 +36,7 @@ namespace { // Simply adding a StringToNumber for longs doesn't work since sometimes long // and int64_t are actually the same type, resulting in a redefinition error. template -bool LocalStringToNumber(const base::StringPiece& string, Type* number) { +bool LocalStringToNumber(const std::string& string, Type* number) { static_assert(sizeof(Type) == sizeof(int) || sizeof(Type) == sizeof(int64_t), "Unexpected Type size"); diff --git a/util/mach/symbolic_constants_mach.cc b/util/mach/symbolic_constants_mach.cc index b7296be2..8f95915e 100644 --- a/util/mach/symbolic_constants_mach.cc +++ b/util/mach/symbolic_constants_mach.cc @@ -239,7 +239,8 @@ bool StringToException(const base::StringPiece& string, } if (options & kAllowNumber) { - return StringToNumber(string, reinterpret_cast(exception)); + return StringToNumber(std::string(string.data(), string.length()), + reinterpret_cast(exception)); } return false; @@ -352,7 +353,7 @@ bool StringToExceptionMask(const base::StringPiece& string, } if (options & kAllowNumber) { - return StringToNumber(string, + return StringToNumber(std::string(string.data(), string.length()), reinterpret_cast(exception_mask)); } @@ -452,7 +453,8 @@ bool StringToExceptionBehavior(const base::StringPiece& string, if (options & kAllowNumber) { exception_behavior_t temp_behavior; - if (!StringToNumber(sp, reinterpret_cast(&temp_behavior))) { + if (!StringToNumber(std::string(sp.data(), sp.length()), + reinterpret_cast(&temp_behavior))) { return false; } build_behavior |= temp_behavior; @@ -539,7 +541,8 @@ bool StringToThreadStateFlavor(const base::StringPiece& string, } if (options & kAllowNumber) { - return StringToNumber(string, reinterpret_cast(flavor)); + return StringToNumber(std::string(string.data(), string.length()), + reinterpret_cast(flavor)); } return false; diff --git a/util/posix/symbolic_constants_posix.cc b/util/posix/symbolic_constants_posix.cc index 51cc583b..c973c146 100644 --- a/util/posix/symbolic_constants_posix.cc +++ b/util/posix/symbolic_constants_posix.cc @@ -161,7 +161,7 @@ bool StringToSignal(const base::StringPiece& string, } if (options & kAllowNumber) { - return StringToNumber(string, signal); + return StringToNumber(std::string(string.data(), string.length()), signal); } return false; diff --git a/util/stdlib/string_number_conversion.cc b/util/stdlib/string_number_conversion.cc index c3352bee..859a8333 100644 --- a/util/stdlib/string_number_conversion.cc +++ b/util/stdlib/string_number_conversion.cc @@ -116,7 +116,7 @@ struct StringToUnsignedInt64Traits }; template -bool StringToIntegerInternal(const base::StringPiece& string, +bool StringToIntegerInternal(const std::string& string, typename Traits::IntType* number) { using IntType = typename Traits::IntType; using LongType = typename Traits::LongType; @@ -127,14 +127,6 @@ bool StringToIntegerInternal(const base::StringPiece& string, return false; } - if (string[string.length()] != '\0') { - // The implementations use the C standard library’s conversion routines, - // which rely on the strings having a trailing NUL character. std::string - // will NUL-terminate. - std::string terminated_string(string.data(), string.length()); - return StringToIntegerInternal(terminated_string, number); - } - errno = 0; char* end; LongType result = Traits::Convert(string.data(), &end, 0); @@ -152,19 +144,19 @@ bool StringToIntegerInternal(const base::StringPiece& string, namespace crashpad { -bool StringToNumber(const base::StringPiece& string, int* number) { +bool StringToNumber(const std::string& string, int* number) { return StringToIntegerInternal(string, number); } -bool StringToNumber(const base::StringPiece& string, unsigned int* number) { +bool StringToNumber(const std::string& string, unsigned int* number) { return StringToIntegerInternal(string, number); } -bool StringToNumber(const base::StringPiece& string, int64_t* number) { +bool StringToNumber(const std::string& string, int64_t* number) { return StringToIntegerInternal(string, number); } -bool StringToNumber(const base::StringPiece& string, uint64_t* number) { +bool StringToNumber(const std::string& 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 b7bdcce8..b5f1d44a 100644 --- a/util/stdlib/string_number_conversion.h +++ b/util/stdlib/string_number_conversion.h @@ -15,7 +15,7 @@ #ifndef CRASHPAD_UTIL_STDLIB_STRING_NUMBER_CONVERSION_H_ #define CRASHPAD_UTIL_STDLIB_STRING_NUMBER_CONVERSION_H_ -#include "base/strings/string_piece.h" +#include namespace crashpad { @@ -54,10 +54,10 @@ namespace crashpad { //! allow arbitrary bases based on whether the string begins with a prefix //! indicating its base. The functions here are provided for situations //! 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); +bool StringToNumber(const std::string& string, int* number); +bool StringToNumber(const std::string& string, unsigned int* number); +bool StringToNumber(const std::string& string, int64_t* number); +bool StringToNumber(const std::string& string, uint64_t* number); //! \} } // namespace crashpad diff --git a/util/stdlib/string_number_conversion_test.cc b/util/stdlib/string_number_conversion_test.cc index dd17c00d..d855c8d7 100644 --- a/util/stdlib/string_number_conversion_test.cc +++ b/util/stdlib/string_number_conversion_test.cc @@ -114,13 +114,9 @@ TEST(StringNumberConversion, StringToInt) { // is split to avoid MSVC warning: // "decimal digit terminates octal escape sequence". static constexpr char input[] = "6\000" "6"; - base::StringPiece input_string(input, arraysize(input) - 1); + std::string input_string(input, arraysize(input) - 1); int output; EXPECT_FALSE(StringToNumber(input_string, &output)); - - // Ensure that a NUL is not required at the end of the string. - EXPECT_TRUE(StringToNumber(base::StringPiece("66", 1), &output)); - EXPECT_EQ(output, 6); } TEST(StringNumberConversion, StringToUnsignedInt) { @@ -212,13 +208,9 @@ TEST(StringNumberConversion, StringToUnsignedInt) { // is split to avoid MSVC warning: // "decimal digit terminates octal escape sequence". static constexpr char input[] = "6\000" "6"; - base::StringPiece input_string(input, arraysize(input) - 1); + std::string input_string(input, arraysize(input) - 1); unsigned int output; EXPECT_FALSE(StringToNumber(input_string, &output)); - - // Ensure that a NUL is not required at the end of the string. - EXPECT_TRUE(StringToNumber(base::StringPiece("66", 1), &output)); - EXPECT_EQ(output, 6u); } TEST(StringNumberConversion, StringToInt64) {