Skip to content

[libc++] Diagnose when nullptrs are passed to string APIs #122790

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

philnik777
Copy link
Contributor

@philnik777 philnik777 commented Jan 13, 2025

This allows catching misuses of APIs that take a pointer to a null-terminated string.

@philnik777
Copy link
Contributor Author

CC @nico

Copy link

github-actions bot commented Jan 13, 2025

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r ec9c2935e19171ce8004e1d970f9b7bf068d92a7...5354045d0e3201507121104b22d828fa4cafe517 libcxx/utils/libcxx/test/params.py
View the diff from darker here.
--- params.py	2025-02-26 10:04:09.000000 +0000
+++ params.py	2025-02-26 10:07:17.516951 +0000
@@ -70,11 +70,10 @@
     "-Wno-mismatched-new-delete",
     "-Wno-redundant-move",
 
     # This doesn't make sense in real code, but we have to test it because the standard requires us to not break
     "-Wno-self-move",
-
     # We're not annotating all the APIs, since that's a lot of annotations compared to how many we actually care about
     "-Wno-nullability-completeness",
 ]
 
 _allStandards = ["c++03", "c++11", "c++14", "c++17", "c++20", "c++23", "c++26"]

@philnik777 philnik777 force-pushed the string_diagnose_nullptr branch from ab8a7bc to b49e718 Compare January 16, 2025 11:23
Copy link

github-actions bot commented Jan 16, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@philnik777 philnik777 force-pushed the string_diagnose_nullptr branch from b49e718 to 2e6e4bb Compare February 22, 2025 10:02
@philnik777 philnik777 marked this pull request as ready for review February 22, 2025 10:02
@philnik777 philnik777 requested a review from a team as a code owner February 22, 2025 10:02
@llvmbot llvmbot added cmake Build system in general and CMake in particular libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. labels Feb 22, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 22, 2025

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This allows catching misuses of APIs that take a pointer to a null-terminated string.


Full diff: https://github.com/llvm/llvm-project/pull/122790.diff

5 Files Affected:

  • (modified) libcxx/include/__config (+6)
  • (modified) libcxx/include/string (+21-17)
  • (added) libcxx/test/libcxx/strings/basic.string/nonnull.verify.cpp (+41)
  • (modified) libcxx/utils/libcxx/test/params.py (+3)
  • (modified) runtimes/cmake/Modules/WarningFlags.cmake (+1)
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 53900e40655ef..5ed36d210e1e5 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -1201,6 +1201,12 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK
 #  endif
 
+#  if __has_feature(nullability)
+#    define _LIBCPP_DIAGNOSE_NONNULL _Nonnull
+#  else
+#    define _LIBCPP_DIAGNOSE_NONNULL
+#  endif
+
 // TODO(LLVM 22): Remove this macro once LLVM19 support ends. __cpp_explicit_this_parameter has been set in LLVM20.
 // Clang-18 has support for deducing this, but it does not set the FTM.
 #  if defined(__cpp_explicit_this_parameter) || (defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER >= 1800)
diff --git a/libcxx/include/string b/libcxx/include/string
index e2968621bb314..2e03159bb6990 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1037,13 +1037,14 @@ public:
 #  endif // _LIBCPP_CXX03_LANG
 
   template <__enable_if_t<__is_allocator<_Allocator>::value, int> = 0>
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const _CharT* __s) {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const _CharT* _LIBCPP_DIAGNOSE_NONNULL __s) {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "basic_string(const char*) detected nullptr");
     __init(__s, traits_type::length(__s));
   }
 
   template <__enable_if_t<__is_allocator<_Allocator>::value, int> = 0>
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const _CharT* __s, const _Allocator& __a)
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
+  basic_string(const _CharT* _LIBCPP_DIAGNOSE_NONNULL __s, const _Allocator& __a)
       : __alloc_(__a) {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "basic_string(const char*, allocator) detected nullptr");
     __init(__s, traits_type::length(__s));
@@ -1214,7 +1215,8 @@ public:
     return assign(__il.begin(), __il.size());
   }
 #  endif
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& operator=(const value_type* __s) {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string&
+  operator=(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s) {
     return assign(__s);
   }
 #  if _LIBCPP_STD_VER >= 23
@@ -1332,7 +1334,8 @@ public:
     return append(__sv);
   }
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& operator+=(const value_type* __s) {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string&
+  operator+=(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s) {
     return append(__s);
   }
 
@@ -1373,7 +1376,7 @@ public:
   append(const _Tp& __t, size_type __pos, size_type __n = npos);
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* __s, size_type __n);
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* __s);
+  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s);
   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(size_type __n, value_type __c);
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __append_default_init(size_type __n);
@@ -1531,7 +1534,7 @@ public:
   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string&
   insert(size_type __pos1, const basic_string& __str, size_type __pos2, size_type __n = npos);
   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& insert(size_type __pos, const value_type* __s, size_type __n);
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& insert(size_type __pos, const value_type* __s);
+  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& insert(size_type __pos, const value_type* _LIBCPP_DIAGNOSE_NONNULL __s);
   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& insert(size_type __pos, size_type __n, value_type __c);
   _LIBCPP_CONSTEXPR_SINCE_CXX20 iterator insert(const_iterator __pos, value_type __c);
 
@@ -1711,7 +1714,7 @@ public:
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type
-  find(const value_type* __s, size_type __pos = 0) const _NOEXCEPT {
+  find(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s, size_type __pos = 0) const _NOEXCEPT {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "string::find(): received nullptr");
     return std::__str_find<value_type, size_type, traits_type, npos>(
         data(), size(), __s, __pos, traits_type::length(__s));
@@ -1742,7 +1745,7 @@ public:
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type
-  rfind(const value_type* __s, size_type __pos = npos) const _NOEXCEPT {
+  rfind(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s, size_type __pos = npos) const _NOEXCEPT {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "string::rfind(): received nullptr");
     return std::__str_rfind<value_type, size_type, traits_type, npos>(
         data(), size(), __s, __pos, traits_type::length(__s));
@@ -1775,7 +1778,7 @@ public:
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type
-  find_first_of(const value_type* __s, size_type __pos = 0) const _NOEXCEPT {
+  find_first_of(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s, size_type __pos = 0) const _NOEXCEPT {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "string::find_first_of(): received nullptr");
     return std::__str_find_first_of<value_type, size_type, traits_type, npos>(
         data(), size(), __s, __pos, traits_type::length(__s));
@@ -1809,7 +1812,7 @@ public:
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type
-  find_last_of(const value_type* __s, size_type __pos = npos) const _NOEXCEPT {
+  find_last_of(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s, size_type __pos = npos) const _NOEXCEPT {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "string::find_last_of(): received nullptr");
     return std::__str_find_last_of<value_type, size_type, traits_type, npos>(
         data(), size(), __s, __pos, traits_type::length(__s));
@@ -1843,7 +1846,7 @@ public:
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type
-  find_first_not_of(const value_type* __s, size_type __pos = 0) const _NOEXCEPT {
+  find_first_not_of(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s, size_type __pos = 0) const _NOEXCEPT {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "string::find_first_not_of(): received nullptr");
     return std::__str_find_first_not_of<value_type, size_type, traits_type, npos>(
         data(), size(), __s, __pos, traits_type::length(__s));
@@ -1877,7 +1880,7 @@ public:
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type
-  find_last_not_of(const value_type* __s, size_type __pos = npos) const _NOEXCEPT {
+  find_last_not_of(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s, size_type __pos = npos) const _NOEXCEPT {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "string::find_last_not_of(): received nullptr");
     return std::__str_find_last_not_of<value_type, size_type, traits_type, npos>(
         data(), size(), __s, __pos, traits_type::length(__s));
@@ -1925,12 +1928,13 @@ public:
     return __self_view(*this).substr(__pos1, __n1).compare(__sv.substr(__pos2, __n2));
   }
 
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 int compare(const value_type* __s) const _NOEXCEPT {
+  _LIBCPP_CONSTEXPR_SINCE_CXX20 int compare(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s) const _NOEXCEPT {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "string::compare(): received nullptr");
     return compare(0, npos, __s, traits_type::length(__s));
   }
 
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 int compare(size_type __pos1, size_type __n1, const value_type* __s) const {
+  _LIBCPP_CONSTEXPR_SINCE_CXX20 int
+  compare(size_type __pos1, size_type __n1, const value_type* _LIBCPP_DIAGNOSE_NONNULL __s) const {
     _LIBCPP_ASSERT_NON_NULL(__s != nullptr, "string::compare(): received nullptr");
     return compare(__pos1, __n1, __s, traits_type::length(__s));
   }
@@ -1949,7 +1953,7 @@ public:
     return !empty() && _Traits::eq(front(), __c);
   }
 
-  constexpr _LIBCPP_HIDE_FROM_ABI bool starts_with(const value_type* __s) const noexcept {
+  constexpr _LIBCPP_HIDE_FROM_ABI bool starts_with(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s) const noexcept {
     return starts_with(__self_view(__s));
   }
 
@@ -1963,7 +1967,7 @@ public:
     return !empty() && _Traits::eq(back(), __c);
   }
 
-  constexpr _LIBCPP_HIDE_FROM_ABI bool ends_with(const value_type* __s) const noexcept {
+  constexpr _LIBCPP_HIDE_FROM_ABI bool ends_with(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s) const noexcept {
     return ends_with(__self_view(__s));
   }
 #  endif
@@ -1979,7 +1983,7 @@ public:
     return __self_view(typename __self_view::__assume_valid(), data(), size()).contains(__c);
   }
 
-  constexpr _LIBCPP_HIDE_FROM_ABI bool contains(const value_type* __s) const {
+  constexpr _LIBCPP_HIDE_FROM_ABI bool contains(const value_type* _LIBCPP_DIAGNOSE_NONNULL __s) const {
     return __self_view(typename __self_view::__assume_valid(), data(), size()).contains(__s);
   }
 #  endif
diff --git a/libcxx/test/libcxx/strings/basic.string/nonnull.verify.cpp b/libcxx/test/libcxx/strings/basic.string/nonnull.verify.cpp
new file mode 100644
index 0000000000000..d61896277afd4
--- /dev/null
+++ b/libcxx/test/libcxx/strings/basic.string/nonnull.verify.cpp
@@ -0,0 +1,41 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+
+// Ensure that APIs which take a CharT* (and no size for it) are diagnosing passing a nullptr to them
+
+#include <string>
+
+#include "test_macros.h"
+
+void func() {
+  const char* const np = nullptr;
+  std::string str1(np);                         // expected-warning {{null passed}}
+  std::string str2(np, std::allocator<char>{}); // expected-warning {{null passed}}
+  str2 = np;                                    // expected-warning {{null passed}}
+  str2 += np;                                   // expected-warning {{null passed}}
+  str2.append(np);                              // expected-warning {{null passed}}
+  str2.insert(0, np);                           // expected-warning {{null passed}}
+  str2.find(np);                                // expected-warning {{null passed}}
+  str2.rfind(np);                               // expected-warning {{null passed}}
+  str2.find_first_of(np);                       // expected-warning {{null passed}}
+  str2.find_last_of(np);                        // expected-warning {{null passed}}
+  str2.find_first_not_of(np);                   // expected-warning {{null passed}}
+  str2.find_last_not_of(np);                    // expected-warning {{null passed}}
+  str2.compare(np);                             // expected-warning {{null passed}}
+  str2.compare(0, 0, np);                       // expected-warning {{null passed}}
+
+#if TEST_STD_VER >= 20
+  str2.starts_with(np); // expected-warning {{null passed}}
+  str2.ends_with(np);   // expected-warning {{null passed}}
+#endif
+#if TEST_STD_VER >= 23
+  str2.contains(np); // expected-warning {{null passed}}
+#endif
+}
diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index 7dba39bc5db04..93dc3a8be3f08 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -72,6 +72,9 @@
 
     # This doesn't make sense in real code, but we have to test it because the standard requires us to not break
     "-Wno-self-move",
+
+    # We're not annotating all the APIs, since that's a lot of annotations compared to how many we actually care about
+    "-Wno-nullability-completeness",
 ]
 
 _allStandards = ["c++03", "c++11", "c++14", "c++17", "c++20", "c++23", "c++26"]
diff --git a/runtimes/cmake/Modules/WarningFlags.cmake b/runtimes/cmake/Modules/WarningFlags.cmake
index d17bf92389d0b..43ef76561cc54 100644
--- a/runtimes/cmake/Modules/WarningFlags.cmake
+++ b/runtimes/cmake/Modules/WarningFlags.cmake
@@ -25,6 +25,7 @@ function(cxx_add_warning_flags target enable_werror enable_pedantic)
       -Wformat-nonliteral
       -Wzero-length-array
       -Wdeprecated-redundant-constexpr-static-def
+      -Wno-nullability-completeness
       )
 
   if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

@mordante mordante self-assigned this Feb 25, 2025
@philnik777 philnik777 force-pushed the string_diagnose_nullptr branch from 2e6e4bb to 5354045 Compare February 26, 2025 10:04
Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@philnik777 philnik777 merged commit f896bd3 into llvm:main Feb 27, 2025
78 of 83 checks passed
@philnik777 philnik777 deleted the string_diagnose_nullptr branch February 27, 2025 21:57
@Prabhuk
Copy link
Contributor

Prabhuk commented Mar 4, 2025

We are seeing build errors in our Fuchsia builders which may be related to this PR.

error: pointer is missing a nullability type specifier

[70533/159115](63) CXX obj/BUILD_DIR/gen/zircon/system/ulib/c/include/header-tests/header-tests.cxx11.complex.h.cc.o
FAILED: [code=1] obj/BUILD_DIR/gen/zircon/system/ulib/c/include/header-tests/header-tests.cxx11.complex.h.cc.o
../../prebuilt/third_party/clang/custom/bin/clang++ -MD -MF obj/BUILD_DIR/gen/zircon/system/ulib/c/include/header-tests/header-tests.cxx11.complex.h.cc.o.d -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 -DZX_ASSERT_LEVEL=2 -DLIBC_COPT_USE_C_ASSERT=1 -DLIBC_COPT_TEST_USE_ZXTEST -DLIBC_NAMESPACE=fuchsia_libc_unittest -DZX...
In file included from gen/zircon/system/ulib/c/include/header-tests/complex.h.cc:2:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/complex.h:30:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/complex:273:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/sstream:323:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/__ostream/basic_ostream.h:21:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/__ostream/put_character_sequence.h:19:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/__locale_dir/pad_and_output.h:16:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/ios:223:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/__locale:25:
../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/string:1057:80: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
 1057 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const _CharT* __s, size_type __n) {
      |                                                                                ^
../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/string:1057:80: note: insert '_Nullable' if the pointer may be null
../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/string:1057:80: note: insert '_Nonnull' if the pointer should never be null
../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/string:1063:28: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
 1063 |   basic_string(const _CharT* __s, size_type __n, const _Allocator& __a)
      |                            ^
../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/string:1063:28: note: insert '_Nullable' if the pointer may be null
../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/string:1063:28: note: insert '_Nonnull' if the pointer should never be null
../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/string:1386:70: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-We...

@philnik777
Copy link
Contributor Author

@Prabhuk it looks like you're not including libc++ as system headers and define _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER. You should always include libc++ as system headers and never define _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER, so doing both is definitely not a supported configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants