Commit f555f69b by Martin Brænne Committed by Copybara-Service

LSC: Fix null safety issues diagnosed by Clang’s `-Wnonnull` and `-Wnullability`.

**Note**: These changes are generated by hand. Review with care.

This CL changes the `absl_internal` implementations of `AsciiStrToLower()` /
`AsciiStrToUpper()` to allow `src` to be null. This can, in fact, happen if the
`string_view` passed to the public API is empty, and the implementations handle
it correctly. I have added comments noting that `src` is allowed to be null
iff the size is zero.

`-Wnonnull` diagnoses cases where a `nullptr` literal is passed to a parameter
annotated nonnull, or where `nullptr` is returned from a function whose return
type is annotated nonnull.

`-Wnullability` diagnoses cases where nullability annotations conflict, for
example between the declaration and definition of a function.

PiperOrigin-RevId: 673846759
Change-Id: I6cf3490ce13837eba9814156c420598000ecc596
parent a1a7086e
...@@ -178,9 +178,10 @@ constexpr bool AsciiInAZRange(unsigned char c) { ...@@ -178,9 +178,10 @@ constexpr bool AsciiInAZRange(unsigned char c) {
} }
// Force-inline so the compiler won't merge the short and long implementations. // Force-inline so the compiler won't merge the short and long implementations.
// `src` may be null iff `size` is zero.
template <bool ToUpper> template <bool ToUpper>
ABSL_ATTRIBUTE_ALWAYS_INLINE inline constexpr void AsciiStrCaseFoldImpl( ABSL_ATTRIBUTE_ALWAYS_INLINE inline constexpr void AsciiStrCaseFoldImpl(
absl::Nonnull<char*> dst, absl::Nonnull<const char*> src, size_t size) { absl::Nonnull<char*> dst, absl::Nullable<const char*> src, size_t size) {
// The upper- and lowercase versions of ASCII characters differ by only 1 bit. // The upper- and lowercase versions of ASCII characters differ by only 1 bit.
// When we need to flip the case, we can xor with this bit to achieve the // When we need to flip the case, we can xor with this bit to achieve the
// desired result. Note that the choice of 'a' and 'A' here is arbitrary. We // desired result. Note that the choice of 'a' and 'A' here is arbitrary. We
...@@ -199,28 +200,30 @@ ABSL_ATTRIBUTE_ALWAYS_INLINE inline constexpr void AsciiStrCaseFoldImpl( ...@@ -199,28 +200,30 @@ ABSL_ATTRIBUTE_ALWAYS_INLINE inline constexpr void AsciiStrCaseFoldImpl(
constexpr size_t kCaseFoldThreshold = 16; constexpr size_t kCaseFoldThreshold = 16;
// No-inline so the compiler won't merge the short and long implementations. // No-inline so the compiler won't merge the short and long implementations.
// `src` may be null iff `size` is zero.
template <bool ToUpper> template <bool ToUpper>
ABSL_ATTRIBUTE_NOINLINE constexpr void AsciiStrCaseFoldLong( ABSL_ATTRIBUTE_NOINLINE constexpr void AsciiStrCaseFoldLong(
absl::Nonnull<char*> dst, absl::Nonnull<const char*> src, size_t size) { absl::Nonnull<char*> dst, absl::Nullable<const char*> src, size_t size) {
ABSL_ASSUME(size >= kCaseFoldThreshold); ABSL_ASSUME(size >= kCaseFoldThreshold);
AsciiStrCaseFoldImpl<ToUpper>(dst, src, size); AsciiStrCaseFoldImpl<ToUpper>(dst, src, size);
} }
// Splitting to short and long strings to allow vectorization decisions // Splitting to short and long strings to allow vectorization decisions
// to be made separately in the long and short cases. // to be made separately in the long and short cases.
// `src` may be null iff `size` is zero.
template <bool ToUpper> template <bool ToUpper>
constexpr void AsciiStrCaseFold(absl::Nonnull<char*> dst, constexpr void AsciiStrCaseFold(absl::Nonnull<char*> dst,
absl::Nonnull<const char*> src, size_t size) { absl::Nullable<const char*> src, size_t size) {
size < kCaseFoldThreshold ? AsciiStrCaseFoldImpl<ToUpper>(dst, src, size) size < kCaseFoldThreshold ? AsciiStrCaseFoldImpl<ToUpper>(dst, src, size)
: AsciiStrCaseFoldLong<ToUpper>(dst, src, size); : AsciiStrCaseFoldLong<ToUpper>(dst, src, size);
} }
void AsciiStrToLower(absl::Nonnull<char*> dst, absl::Nonnull<const char*> src, void AsciiStrToLower(absl::Nonnull<char*> dst, absl::Nullable<const char*> src,
size_t n) { size_t n) {
return AsciiStrCaseFold<false>(dst, src, n); return AsciiStrCaseFold<false>(dst, src, n);
} }
void AsciiStrToUpper(absl::Nonnull<char*> dst, absl::Nonnull<const char*> src, void AsciiStrToUpper(absl::Nonnull<char*> dst, absl::Nullable<const char*> src,
size_t n) { size_t n) {
return AsciiStrCaseFold<true>(dst, src, n); return AsciiStrCaseFold<true>(dst, src, n);
} }
......
...@@ -76,10 +76,10 @@ ABSL_DLL extern const char kToUpper[256]; ...@@ -76,10 +76,10 @@ ABSL_DLL extern const char kToUpper[256];
// Declaration for the array of characters to lower-case characters. // Declaration for the array of characters to lower-case characters.
ABSL_DLL extern const char kToLower[256]; ABSL_DLL extern const char kToLower[256];
void AsciiStrToLower(absl::Nonnull<char*> dst, absl::Nonnull<const char*> src, void AsciiStrToLower(absl::Nonnull<char*> dst, absl::Nullable<const char*> src,
size_t n); size_t n);
void AsciiStrToUpper(absl::Nonnull<char*> dst, absl::Nonnull<const char*> src, void AsciiStrToUpper(absl::Nonnull<char*> dst, absl::Nullable<const char*> src,
size_t n); size_t n);
} // namespace ascii_internal } // namespace ascii_internal
......
...@@ -200,6 +200,10 @@ TEST(AsciiStrTo, Lower) { ...@@ -200,6 +200,10 @@ TEST(AsciiStrTo, Lower) {
EXPECT_EQ("abcdefghijklmnopqrstuvwxyz1!a", absl::AsciiStrToLower(long_str)); EXPECT_EQ("abcdefghijklmnopqrstuvwxyz1!a", absl::AsciiStrToLower(long_str));
EXPECT_EQ("pqrstu", absl::AsciiStrToLower(fun())); EXPECT_EQ("pqrstu", absl::AsciiStrToLower(fun()));
// An empty `string_view` specifically exercises the case where a null data
// pointer is passed to internal functions.
EXPECT_EQ("", absl::AsciiStrToLower(absl::string_view()));
absl::AsciiStrToLower(&mutable_str); absl::AsciiStrToLower(&mutable_str);
EXPECT_EQ("_`?@[{amnopqrstuvwxyz", mutable_str); EXPECT_EQ("_`?@[{amnopqrstuvwxyz", mutable_str);
...@@ -223,6 +227,10 @@ TEST(AsciiStrTo, Upper) { ...@@ -223,6 +227,10 @@ TEST(AsciiStrTo, Upper) {
EXPECT_EQ("ABCDEFGHIJKLMNOPQRSTUVWXYZ1!A", absl::AsciiStrToUpper(long_str)); EXPECT_EQ("ABCDEFGHIJKLMNOPQRSTUVWXYZ1!A", absl::AsciiStrToUpper(long_str));
EXPECT_EQ("PQRSTU", absl::AsciiStrToUpper(fun())); EXPECT_EQ("PQRSTU", absl::AsciiStrToUpper(fun()));
// An empty `string_view` specifically exercises the case where a null data
// pointer is passed to internal functions.
EXPECT_EQ("", absl::AsciiStrToUpper(absl::string_view()));
char mutable_buf[] = "Mutable"; char mutable_buf[] = "Mutable";
std::transform(mutable_buf, mutable_buf + strlen(mutable_buf), std::transform(mutable_buf, mutable_buf + strlen(mutable_buf),
mutable_buf, absl::ascii_toupper); mutable_buf, absl::ascii_toupper);
......
...@@ -516,7 +516,13 @@ TEST_F(FormatEntryPointTest, SNPrintF) { ...@@ -516,7 +516,13 @@ TEST_F(FormatEntryPointTest, SNPrintF) {
EXPECT_EQ(result, 17); EXPECT_EQ(result, 17);
EXPECT_EQ(std::string(buffer), "NUMBER: 1234567"); EXPECT_EQ(std::string(buffer), "NUMBER: 1234567");
result = SNPrintF(nullptr, 0, "Just checking the %s of the output.", "size"); // The `output` parameter is annotated nonnull, but we want to test that
// it is never written to if the size is zero.
// Use a variable instead of passing nullptr directly to avoid a `-Wnonnull`
// warning.
char* null_output = nullptr;
result =
SNPrintF(null_output, 0, "Just checking the %s of the output.", "size");
EXPECT_EQ(result, 37); EXPECT_EQ(result, 37);
} }
...@@ -545,7 +551,13 @@ TEST_F(FormatEntryPointTest, SNPrintFWithV) { ...@@ -545,7 +551,13 @@ TEST_F(FormatEntryPointTest, SNPrintFWithV) {
std::string size = "size"; std::string size = "size";
result = SNPrintF(nullptr, 0, "Just checking the %v of the output.", size); // The `output` parameter is annotated nonnull, but we want to test that
// it is never written to if the size is zero.
// Use a variable instead of passing nullptr directly to avoid a `-Wnonnull`
// warning.
char* null_output = nullptr;
result =
SNPrintF(null_output, 0, "Just checking the %v of the output.", size);
EXPECT_EQ(result, 37); EXPECT_EQ(result, 37);
} }
......
...@@ -894,7 +894,11 @@ TEST(StringViewTest, NULLInput) { ...@@ -894,7 +894,11 @@ TEST(StringViewTest, NULLInput) {
EXPECT_EQ(s.size(), 0u); EXPECT_EQ(s.size(), 0u);
#ifdef ABSL_HAVE_STRING_VIEW_FROM_NULLPTR #ifdef ABSL_HAVE_STRING_VIEW_FROM_NULLPTR
s = absl::string_view(nullptr); // The `str` parameter is annotated nonnull, but we want to test the defensive
// null check. Use a variable instead of passing nullptr directly to avoid a
// `-Wnonnull` warning.
char* null_str = nullptr;
s = absl::string_view(null_str);
EXPECT_EQ(s.data(), nullptr); EXPECT_EQ(s.data(), nullptr);
EXPECT_EQ(s.size(), 0u); EXPECT_EQ(s.size(), 0u);
...@@ -1076,9 +1080,22 @@ TEST(StringViewTest, ConstexprNullSafeStringView) { ...@@ -1076,9 +1080,22 @@ TEST(StringViewTest, ConstexprNullSafeStringView) {
TEST(StringViewTest, ConstexprCompiles) { TEST(StringViewTest, ConstexprCompiles) {
constexpr absl::string_view sp; constexpr absl::string_view sp;
// With `-Wnonnull` turned on, there is no way to test the defensive null
// check in the `string_view(const char*)` constructor in a constexpr context,
// as the argument needs to be constexpr. The compiler will therefore always
// know at compile time that the argument is nullptr and complain because the
// parameter is annotated nonnull. We hence turn the warning off for this
// test.
#if defined(__clang__)
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wnonnull"
#endif
#ifdef ABSL_HAVE_STRING_VIEW_FROM_NULLPTR #ifdef ABSL_HAVE_STRING_VIEW_FROM_NULLPTR
constexpr absl::string_view cstr(nullptr); constexpr absl::string_view cstr(nullptr);
#endif #endif
#if defined(__clang__)
#pragma clang diagnostic pop
#endif
constexpr absl::string_view cstr_len("cstr", 4); constexpr absl::string_view cstr_len("cstr", 4);
#if defined(ABSL_USES_STD_STRING_VIEW) #if defined(ABSL_USES_STD_STRING_VIEW)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment