Skip to content

Commit

Permalink
Avoid storing values >= 0x80 in "char"s.
Browse files Browse the repository at this point in the history 
Since char is signed, this causes -Wc++11-narrowing to fire.  Instead
use uint8_t[], and then either cast to const char* or replace
StringPiece usage with span<const uint8_t>.

Bug: 1216696
Change-Id: I15086cb60f55096a4b6bc81a07ba06be4ac7d62d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2951642
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#892058}
  • Loading branch information
@pkasting
pkasting authored and Chromium LUCI CQ committed Jun 14, 2021
1 parent 85b27f4 commit 0ff39d4
Show file tree
Hide file tree
Showing 30 changed files with 221 additions and 187 deletions.
22 changes: 11 additions & 11 deletions base/big_endian_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
namespace base {

TEST(ReadBigEndianTest, ReadSignedPositive) {
char data[] = {0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, 0x1A, 0X2A};
char data[] = {0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, 0x1A, 0x2A};
int8_t s8 = 0;
int16_t s16 = 0;
int32_t s32 = 0;
Expand All @@ -30,31 +30,31 @@ TEST(ReadBigEndianTest, ReadSignedPositive) {
}

TEST(ReadBigEndianTest, ReadSignedNegative) {
char data[] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0XFF};
uint8_t data[] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
int8_t s8 = 0;
int16_t s16 = 0;
int32_t s32 = 0;
int64_t s64 = 0;
ReadBigEndian(data, &s8);
ReadBigEndian(data, &s16);
ReadBigEndian(data, &s32);
ReadBigEndian(data, &s64);
ReadBigEndian(reinterpret_cast<const char*>(data), &s8);
ReadBigEndian(reinterpret_cast<const char*>(data), &s16);
ReadBigEndian(reinterpret_cast<const char*>(data), &s32);
ReadBigEndian(reinterpret_cast<const char*>(data), &s64);
EXPECT_EQ(-1, s8);
EXPECT_EQ(-1, s16);
EXPECT_EQ(-1, s32);
EXPECT_EQ(-1, s64);
}

TEST(ReadBigEndianTest, ReadUnsignedSigned) {
char data[] = {0xA0, 0xB0, 0xC0, 0xD0, 0xE0, 0xF0, 0xA1, 0XA2};
uint8_t data[] = {0xA0, 0xB0, 0xC0, 0xD0, 0xE0, 0xF0, 0xA1, 0xA2};
uint8_t u8 = 0;
uint16_t u16 = 0;
uint32_t u32 = 0;
uint64_t u64 = 0;
ReadBigEndian(data, &u8);
ReadBigEndian(data, &u16);
ReadBigEndian(data, &u32);
ReadBigEndian(data, &u64);
ReadBigEndian(reinterpret_cast<const char*>(data), &u8);
ReadBigEndian(reinterpret_cast<const char*>(data), &u16);
ReadBigEndian(reinterpret_cast<const char*>(data), &u32);
ReadBigEndian(reinterpret_cast<const char*>(data), &u64);
EXPECT_EQ(0xA0, u8);
EXPECT_EQ(0xA0B0, u16);
EXPECT_EQ(0xA0B0C0D0, u32);
Expand Down
8 changes: 5 additions & 3 deletions chrome/browser/extensions/api/image_writer_private/xz_extractor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ constexpr base::FilePath::StringPieceType kExtractedBinFileName =
FILE_PATH_LITERAL("extracted.bin");

// https://tukaani.org/xz/xz-file-format-1.0.4.txt
constexpr char kExpectedMagic[6] = {0xfd, '7', 'z', 'X', 'Z', 0x00};
constexpr uint8_t kExpectedMagic[6] = {0xfd, '7', 'z', 'X', 'Z', 0x00};

} // namespace

Expand All @@ -46,8 +46,10 @@ bool XzExtractor::IsXzFile(const base::FilePath& image_path) {
if (infile.ReadAtCurrentPos(actual_magic, kExpectedSize) != kExpectedSize)
return false;

return std::equal(kExpectedMagic, kExpectedMagic + kExpectedSize,
actual_magic);
return std::equal(
reinterpret_cast<const char*>(kExpectedMagic),
reinterpret_cast<const char*>(kExpectedMagic + kExpectedSize),
actual_magic);
}

// static
Expand Down
61 changes: 39 additions & 22 deletions chrome/browser/ssl/sct_reporting_service_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,18 @@ namespace {
// log and one from a non-Google log.
//
// Google's "Argon2023" log ("6D7Q2j71BjUy51covIlryQPTy9ERa+zraeF3fW0GvW4="):
const char kTestGoogleLogId[] = {
const uint8_t kTestGoogleLogId[] = {
0xe8, 0x3e, 0xd0, 0xda, 0x3e, 0xf5, 0x06, 0x35, 0x32, 0xe7, 0x57,
0x28, 0xbc, 0x89, 0x6b, 0xc9, 0x03, 0xd3, 0xcb, 0xd1, 0x11, 0x6b,
0xec, 0xeb, 0x69, 0xe1, 0x77, 0x7d, 0x6d, 0x06, 0xbd, 0x6e};
// Cloudflare's "Nimbus2023" log
// ("ejKMVNi3LbYg6jjgUh7phBZwMhOFTTvSK8E6V6NS61I="):
const char kTestNonGoogleLogId1[] = {
const uint8_t kTestNonGoogleLogId1[] = {
0x7a, 0x32, 0x8c, 0x54, 0xd8, 0xb7, 0x2d, 0xb6, 0x20, 0xea, 0x38,
0xe0, 0x52, 0x1e, 0xe9, 0x84, 0x16, 0x70, 0x32, 0x13, 0x85, 0x4d,
0x3b, 0xd2, 0x2b, 0xc1, 0x3a, 0x57, 0xa3, 0x52, 0xeb, 0x52};
// DigiCert's "Yeti2023" log ("Nc8ZG7+xbFe/D61MbULLu7YnICZR6j/hKu+oA8M71kw="):
const char kTestNonGoogleLogId2[] = {
const uint8_t kTestNonGoogleLogId2[] = {
0x35, 0xcf, 0x19, 0x1b, 0xbf, 0xb1, 0x6c, 0x57, 0xbf, 0x0f, 0xad,
0x4c, 0x6d, 0x42, 0xcb, 0xbb, 0xb6, 0x27, 0x20, 0x26, 0x51, 0xea,
0x3f, 0xe1, 0x2a, 0xef, 0xa8, 0x03, 0xc3, 0x3b, 0xd6, 0x4c};
Expand Down Expand Up @@ -135,17 +135,20 @@ class SCTReportingServiceBrowserTest : public CertVerifierBrowserTest {
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions1",
"signature1", base::Time::Now(),
std::string(kTestGoogleLogId, base::size(kTestGoogleLogId)),
std::string(reinterpret_cast<const char*>(kTestGoogleLogId),
base::size(kTestGoogleLogId)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions2",
"signature2", base::Time::Now(),
std::string(kTestNonGoogleLogId1, base::size(kTestNonGoogleLogId1)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId1),
base::size(kTestNonGoogleLogId1)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions3",
"signature3", base::Time::Now(),
std::string(kTestNonGoogleLogId2, base::size(kTestNonGoogleLogId2)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId2),
base::size(kTestNonGoogleLogId2)),
net::ct::SCT_STATUS_OK, &verify_result.scts);

// Set up two test hosts as using publicly-issued certificates for testing.
Expand Down Expand Up @@ -398,17 +401,20 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions1",
"signature1", base::Time::Now(),
std::string(kTestGoogleLogId, base::size(kTestGoogleLogId)),
std::string(reinterpret_cast<const char*>(kTestGoogleLogId),
base::size(kTestGoogleLogId)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions2",
"signature2", base::Time::Now(),
std::string(kTestNonGoogleLogId1, base::size(kTestNonGoogleLogId1)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId1),
base::size(kTestNonGoogleLogId1)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions3",
"signature3", base::Time::Now(),
std::string(kTestNonGoogleLogId2, base::size(kTestNonGoogleLogId2)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId2),
base::size(kTestNonGoogleLogId2)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
mock_cert_verifier()->AddResultForCertAndHost(
https_server()->GetCertificate().get(), "a.test", verify_result, net::OK);
Expand All @@ -435,24 +441,29 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
verify_result.is_issued_by_known_root = true;
// Add three valid SCTs and one invalid SCT. The three valid SCTs meet the
// Chrome CT policy.
MakeTestSCTAndStatus(net::ct::SignedCertificateTimestamp::SCT_EMBEDDED,
"extensions1", "signature1", base::Time::Now(),
std::string(kTestGoogleLogId, sizeof(kTestGoogleLogId)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions1",
"signature1", base::Time::Now(),
std::string(reinterpret_cast<const char*>(kTestGoogleLogId),
sizeof(kTestGoogleLogId)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions2",
"signature2", base::Time::Now(),
std::string(kTestNonGoogleLogId1, sizeof(kTestNonGoogleLogId1)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId1),
sizeof(kTestNonGoogleLogId1)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions3",
"signature3", base::Time::Now(),
std::string(kTestNonGoogleLogId2, sizeof(kTestNonGoogleLogId2)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId2),
sizeof(kTestNonGoogleLogId2)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions4",
"signature4", base::Time::Now(),
std::string(kTestNonGoogleLogId2, sizeof(kTestNonGoogleLogId2)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId2),
sizeof(kTestNonGoogleLogId2)),
net::ct::SCT_STATUS_INVALID_SIGNATURE, &verify_result.scts);

mock_cert_verifier()->AddResultForCertAndHost(
Expand Down Expand Up @@ -482,17 +493,20 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions1",
"signature1", base::Time::Now(),
std::string(kTestNonGoogleLogId1, sizeof(kTestNonGoogleLogId1)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId1),
sizeof(kTestNonGoogleLogId1)),
net::ct::SCT_STATUS_OK, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions2",
"signature2", base::Time::Now(),
std::string(kTestNonGoogleLogId1, sizeof(kTestNonGoogleLogId1)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId1),
sizeof(kTestNonGoogleLogId1)),
net::ct::SCT_STATUS_INVALID_SIGNATURE, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions3",
"signature3", base::Time::Now(),
std::string(kTestNonGoogleLogId2, sizeof(kTestNonGoogleLogId2)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId2),
sizeof(kTestNonGoogleLogId2)),
net::ct::SCT_STATUS_INVALID_SIGNATURE, &verify_result.scts);

mock_cert_verifier()->AddResultForCertAndHost(
Expand All @@ -517,17 +531,20 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest, NoValidSCTsNoReport) {
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions1",
"signature1", base::Time::Now(),
std::string(kTestNonGoogleLogId1, sizeof(kTestNonGoogleLogId1)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId1),
sizeof(kTestNonGoogleLogId1)),
net::ct::SCT_STATUS_INVALID_TIMESTAMP, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions2",
"signature2", base::Time::Now(),
std::string(kTestNonGoogleLogId1, sizeof(kTestNonGoogleLogId1)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId1),
sizeof(kTestNonGoogleLogId1)),
net::ct::SCT_STATUS_INVALID_SIGNATURE, &verify_result.scts);
MakeTestSCTAndStatus(
net::ct::SignedCertificateTimestamp::SCT_EMBEDDED, "extensions3",
"signature3", base::Time::Now(),
std::string(kTestNonGoogleLogId1, sizeof(kTestNonGoogleLogId1)),
std::string(reinterpret_cast<const char*>(kTestNonGoogleLogId1),
sizeof(kTestNonGoogleLogId1)),
net::ct::SCT_STATUS_INVALID_SIGNATURE, &verify_result.scts);

mock_cert_verifier()->AddResultForCertAndHost(
Expand Down
7 changes: 4 additions & 3 deletions components/bookmarks/browser/bookmark_node_data_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,9 +443,10 @@ TEST_F(BookmarkNodeDataTest, MetaInfo) {
#if !defined(OS_APPLE)
TEST_F(BookmarkNodeDataTest, ReadFromPickleTooManyNodes) {
// Test case determined by a fuzzer. See https://crbug.com/956583.
const char pickled_data[] = {0x08, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0xff, 0x03, 0x03, 0x41};
base::Pickle pickle(pickled_data, sizeof(pickled_data));
const uint8_t pickled_data[] = {0x08, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0xff, 0x03, 0x03, 0x41};
base::Pickle pickle(reinterpret_cast<const char*>(pickled_data),
sizeof(pickled_data));
BookmarkNodeData bookmark_node_data;
EXPECT_FALSE(bookmark_node_data.ReadFromPickle(&pickle));
}
Expand Down
13 changes: 7 additions & 6 deletions components/consent_auditor/consent_auditor_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -349,11 +349,11 @@ TEST_F(ConsentAuditorImplTest, RecordArcPlayConsent) {
play_consent.set_consent_flow(ArcPlayTermsOfServiceConsent::SETUP);

// Verify the hash: 2fd4e1c6 7a2d28fc ed849ee1 bb76e739 1b93eb12.
const char play_tos_hash[] = {0x2f, 0xd4, 0xe1, 0xc6, 0x7a, 0x2d, 0x28,
0xfc, 0xed, 0x84, 0x9e, 0xe1, 0xbb, 0x76,
0xe7, 0x39, 0x1b, 0x93, 0xeb, 0x12};
play_consent.set_play_terms_of_service_hash(
std::string(play_tos_hash, base::kSHA1Length));
const uint8_t play_tos_hash[] = {0x2f, 0xd4, 0xe1, 0xc6, 0x7a, 0x2d, 0x28,
0xfc, 0xed, 0x84, 0x9e, 0xe1, 0xbb, 0x76,
0xe7, 0x39, 0x1b, 0x93, 0xeb, 0x12};
play_consent.set_play_terms_of_service_hash(std::string(
reinterpret_cast<const char*>(play_tos_hash), base::kSHA1Length));
play_consent.set_play_terms_of_service_text_length(7);

consent_auditor()->RecordArcPlayConsent(kAccountId, play_consent);
Expand All @@ -371,7 +371,8 @@ TEST_F(ConsentAuditorImplTest, RecordArcPlayConsent) {
consent.arc_play_terms_of_service_consent();

EXPECT_EQ(7, actual_play_consent.play_terms_of_service_text_length());
EXPECT_EQ(std::string(play_tos_hash, base::kSHA1Length),
EXPECT_EQ(std::string(reinterpret_cast<const char*>(play_tos_hash),
base::kSHA1Length),
actual_play_consent.play_terms_of_service_hash());

EXPECT_EQ(kConfirmationMessageId, actual_play_consent.confirmation_grd_id());
Expand Down
12 changes: 7 additions & 5 deletions components/password_manager/core/browser/leak_detection/encryption_utils_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,13 @@ TEST(EncryptionUtils, CanonicalizeUsername) {
TEST(EncryptionUtils, HashUsername) {
// Same test case as used by the server-side implementation:
// go/passwords-leak-test
constexpr char kExpected[] = {0x3D, 0x70, 0xD3, 0x7B, 0xFC, 0x1A, 0x3D, 0x81,
0x45, 0xE6, 0xC7, 0xA3, 0xA4, 0xD7, 0x92, 0x76,
0x61, 0xC1, 0xE8, 0xDF, 0x82, 0xBD, 0x0C, 0x9F,
0x61, 0x9A, 0xA3, 0xC9, 0x96, 0xEC, 0x4C, 0xB3};
EXPECT_THAT(HashUsername("jonsnow"), ElementsAreArray(kExpected));
constexpr uint8_t kExpected[] = {
0x3D, 0x70, 0xD3, 0x7B, 0xFC, 0x1A, 0x3D, 0x81, 0x45, 0xE6, 0xC7,
0xA3, 0xA4, 0xD7, 0x92, 0x76, 0x61, 0xC1, 0xE8, 0xDF, 0x82, 0xBD,
0x0C, 0x9F, 0x61, 0x9A, 0xA3, 0xC9, 0x96, 0xEC, 0x4C, 0xB3};
EXPECT_THAT(HashUsername("jonsnow"),
ElementsAreArray(reinterpret_cast<const char*>(kExpected),
base::size(kExpected)));
}

TEST(EncryptionUtils, BucketizeUsername) {
Expand Down
9 changes: 5 additions & 4 deletions components/sync/nigori/nigori.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,15 @@ void Nigori::Keys::InitByDerivationUsingPbkdf2(const std::string& password) {
// "dummy") as PBKDF2_HMAC_SHA1(Ns("dummy") + Ns("localhost"), "saltsalt",
// 1001, 128), where Ns(S) is the NigoriStream representation of S (32-bit
// big-endian length of S followed by S itself).
const char kRawConstantSalt[] = {0xc7, 0xca, 0xfb, 0x23, 0xec, 0x2a,
0x9d, 0x4c, 0x03, 0x5a, 0x90, 0xae,
0xed, 0x8b, 0xa4, 0x98};
const uint8_t kRawConstantSalt[] = {0xc7, 0xca, 0xfb, 0x23, 0xec, 0x2a,
0x9d, 0x4c, 0x03, 0x5a, 0x90, 0xae,
0xed, 0x8b, 0xa4, 0x98};
const size_t kUserIterations = 1002;
const size_t kEncryptionIterations = 1003;
const size_t kSigningIterations = 1004;

std::string salt(kRawConstantSalt, sizeof(kRawConstantSalt));
std::string salt(reinterpret_cast<const char*>(kRawConstantSalt),
sizeof(kRawConstantSalt));

// Kuser = PBKDF2(P, Suser, Nuser, 16)
user_key = SymmetricKey::DeriveKeyFromPasswordUsingPbkdf2(
Expand Down
4 changes: 1 addition & 3 deletions headless/lib/headless_content_main_delegate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,7 @@ void InitializeResourceBundle(const base::CommandLine& command_line) {

#ifdef HEADLESS_USE_EMBEDDED_RESOURCES
ui::ResourceBundle::GetSharedInstance().AddDataPackFromBuffer(
base::StringPiece(
reinterpret_cast<const char*>(kHeadlessResourcePak.contents),
kHeadlessResourcePak.length),
{kHeadlessResourcePak.contents, kHeadlessResourcePak.length},
ui::SCALE_FACTOR_NONE);

#else
Expand Down
2 changes: 1 addition & 1 deletion mojo/public/cpp/base/byte_string_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace mojo_base {
TEST(ByteStringTest, Test) {
const std::string kCases[] = {
"hello", // C-string
{0xEF, 0xB7, 0xAF}, // invalid UTF-8
{'\xEF', '\xB7', '\xAF'}, // invalid UTF-8
{'h', '\0', 'w', 'd', 'y'}, // embedded null
};
for (size_t i = 0; i < base::size(kCases); ++i) {
Expand Down

0 comments on commit 0ff39d4

Please sign in to comment.