Don’t read beyond a StringPiece’s bounds in StringToNumber()

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 <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
This commit is contained in:
Mark Mentovai 2018-03-02 21:45:29 -05:00 committed by Commit Bot
parent 82777cff58
commit 23b2156fb6
6 changed files with 21 additions and 34 deletions

View File

@ -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 <typename Type>
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");

View File

@ -239,7 +239,8 @@ bool StringToException(const base::StringPiece& string,
}
if (options & kAllowNumber) {
return StringToNumber(string, reinterpret_cast<unsigned int*>(exception));
return StringToNumber(std::string(string.data(), string.length()),
reinterpret_cast<unsigned int*>(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<unsigned int*>(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<unsigned int*>(&temp_behavior))) {
if (!StringToNumber(std::string(sp.data(), sp.length()),
reinterpret_cast<unsigned int*>(&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<unsigned int*>(flavor));
return StringToNumber(std::string(string.data(), string.length()),
reinterpret_cast<unsigned int*>(flavor));
}
return false;

View File

@ -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;

View File

@ -116,7 +116,7 @@ struct StringToUnsignedInt64Traits
};
template <typename Traits>
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 librarys 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<Traits>(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<StringToIntTraits>(string, number);
}
bool StringToNumber(const base::StringPiece& string, unsigned int* number) {
bool StringToNumber(const std::string& string, unsigned int* number) {
return StringToIntegerInternal<StringToUnsignedIntTraits>(string, number);
}
bool StringToNumber(const base::StringPiece& string, int64_t* number) {
bool StringToNumber(const std::string& string, int64_t* number) {
return StringToIntegerInternal<StringToInt64Traits>(string, number);
}
bool StringToNumber(const base::StringPiece& string, uint64_t* number) {
bool StringToNumber(const std::string& string, uint64_t* number) {
return StringToIntegerInternal<StringToUnsignedInt64Traits>(string, number);
}

View File

@ -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 <string>
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

View File

@ -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) {