Skip to content

Commit

Permalink
LSC: Fix null safety issues diagnosed by Clang’s -Wnonnull and `-Wn…
Browse files Browse the repository at this point in the history
…ullability`.

**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
  • Loading branch information
martinboehme authored and copybara-github committed Sep 12, 2024
1 parent a1a7086 commit f555f69
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 10 deletions.
13 changes: 8 additions & 5 deletions absl/strings/ascii.cc
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,10 @@ constexpr bool AsciiInAZRange(unsigned char c) {
}

// Force-inline so the compiler won't merge the short and long implementations.
// `src` may be null iff `size` is zero.
template <bool ToUpper>
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.
// 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
Expand All @@ -199,28 +200,30 @@ ABSL_ATTRIBUTE_ALWAYS_INLINE inline constexpr void AsciiStrCaseFoldImpl(
constexpr size_t kCaseFoldThreshold = 16;

// No-inline so the compiler won't merge the short and long implementations.
// `src` may be null iff `size` is zero.
template <bool ToUpper>
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);
AsciiStrCaseFoldImpl<ToUpper>(dst, src, size);
}

// Splitting to short and long strings to allow vectorization decisions
// to be made separately in the long and short cases.
// `src` may be null iff `size` is zero.
template <bool ToUpper>
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)
: 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) {
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) {
return AsciiStrCaseFold<true>(dst, src, n);
}
Expand Down
4 changes: 2 additions & 2 deletions absl/strings/ascii.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ ABSL_DLL extern const char kToUpper[256];
// Declaration for the array of characters to lower-case characters.
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);

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

} // namespace ascii_internal
Expand Down
8 changes: 8 additions & 0 deletions absl/strings/ascii_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,10 @@ TEST(AsciiStrTo, Lower) {
EXPECT_EQ("abcdefghijklmnopqrstuvwxyz1!a", absl::AsciiStrToLower(long_str));
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);
EXPECT_EQ("_`?@[{amnopqrstuvwxyz", mutable_str);

Expand All @@ -223,6 +227,10 @@ TEST(AsciiStrTo, Upper) {
EXPECT_EQ("ABCDEFGHIJKLMNOPQRSTUVWXYZ1!A", absl::AsciiStrToUpper(long_str));
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";
std::transform(mutable_buf, mutable_buf + strlen(mutable_buf),
mutable_buf, absl::ascii_toupper);
Expand Down
16 changes: 14 additions & 2 deletions absl/strings/str_format_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,13 @@ TEST_F(FormatEntryPointTest, SNPrintF) {
EXPECT_EQ(result, 17);
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);
}

Expand Down Expand Up @@ -545,7 +551,13 @@ TEST_F(FormatEntryPointTest, SNPrintFWithV) {

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);
}

Expand Down
19 changes: 18 additions & 1 deletion absl/strings/string_view_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -894,7 +894,11 @@ TEST(StringViewTest, NULLInput) {
EXPECT_EQ(s.size(), 0u);

#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.size(), 0u);

Expand Down Expand Up @@ -1076,8 +1080,21 @@ TEST(StringViewTest, ConstexprNullSafeStringView) {

TEST(StringViewTest, ConstexprCompiles) {
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
constexpr absl::string_view cstr(nullptr);
#endif
#if defined(__clang__)
#pragma clang diagnostic pop
#endif
constexpr absl::string_view cstr_len("cstr", 4);

Expand Down

0 comments on commit f555f69

Please sign in to comment.