[-Wunsafe-buffer-usage][BoundsSafety] support __null_terminated in the interop mode #10060

ziqingluo-90 · 2025-02-19T01:11:57Z

For a __null_terminated pointer 'p', only a __null_terminated pointer, a string literal or a call to std::string::c_str can be assigned to (/passed to/used to initialize) 'p'. Otherwise, in the interop mode, an -Wunsafe-buffer-usage warning will be reported.

Similar to how __counted_by is handled, incompatibility in nested __null_terminated pointers (e.g., input/output pointer type arguments) is reported as Bounds Safety type checking errors.

(rdar://138798346)

ziqingluo-90 · 2025-02-19T01:12:12Z

@swift-ci test

ziqingluo-90 · 2025-02-19T01:13:27Z

CC: @dtarditi

ziqingluo-90 · 2025-02-19T01:49:55Z

@swift-ci test

…e interop mode For a __null_terminated pointer 'p', only a __null_terminated pointer, a string literal or a call to `std::string::c_str` can be assigned to (/passed to/used to initialize) 'p'. Otherwise, in the interop mode, an -Wunsafe-buffer-usage warning will be reported. Similar to how __counted_by is handled, incompatibility in nested __null_terminated pointers (e.g., input/output pointer type arguments) is reported as Bounds Safety type checking errors. (rdar://138798346)

ziqingluo-90 · 2025-02-19T19:25:45Z

@swift-ci test

dtarditi

I have some suggestions.

clang/lib/Sema/SemaExpr.cpp

dtarditi · 2025-02-19T19:47:25Z

clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp

+
+#include <ptrcheck.h>
+typedef unsigned size_t;
+


Some of these new tests look unrelated to this change. Please consider splitting them out to a separate PR.

sure. These are things left un-upstreamed.

clang/lib/Sema/SemaExpr.cpp

dtarditi · 2025-02-19T19:52:58Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+                  "initialization of a pointer of|"
+                  "assignment to a pointer of|"
+                  "casting to}2 '__null_terminated' type; "
+  "only '__null_terminated' pointers, string literals, and 'std::string::c_str' calls are compatible with '__null_terminated' pointers">,


It might be simpler to say that. the source pointer may not point to null-terminated data. I could imagine that we eventually we will increase the number of cases that we allow. I don't have super strong opinions about this - it is nice to tell people what is allowed, but it doesn't tell them what the error is.

I see. Yeah, I should explain what the error is. The convention for warning messages (at least for -Wunsafe-buffer-usage) is that we have a short but clear enough sentence to just say "something is unsafe", then put necessary explanation in notes. My message here might already be lengthy. I will explain what the error is in a note and also move "only '__null_terminated' pointers, ..." (sort of the suggestion) to a note too.

oh, I maybe misunderstood part of your comment here.
You were suggesting to simply saying "source pointer may not point to null-terminated data." instead of breaking into cases. I'm not against the idea of making the warning message simpler. My only concern is that warning messages under -Wunsafe-buffer-usage all have the forms "unsafe [some operation]" or "[some operation] is unsafe". I'd like this message to follow that.

Different cases are determined by AssignmentAction, which is handled by the existing framework in Sema. I assume it is stable.

Or maybe "unsafe conversion to __null_terminated type; source pointer may not point to null-terminated data." ? If "conversion" is a appropriate umbrella term for these operations.

hnrklssn · 2025-02-19T23:45:46Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+  "only '__null_terminated' pointers, string literals, and 'std::string::c_str' calls are compatible with '__null_terminated' pointers">,
+  InGroup<UnsafeBufferUsage>, DefaultIgnore;
+def warn_unsafe_named_cast_to_null_terminated_pointer : Warning<
+  "C++ named cast to '__null_terminated' type is unsafe">,


This may be overly specific: any cast to __null_terminated is unsafe. If I got this error message I'd try doing a C-style cast instead, which would not help.

Wait, looking at the test cases it seems like C-style casts are deliberately allowed. What's the rationale?

My original idea is that it should be rare for C++ programmers to cast null_terminated pointers with named casts. And, Bounds Safety's type checking for null_terminated can only be re-used for C-Style casting. So why not force C++ programmers to just use C-Style casts for null_terminated pointers?

But now I changed my mind. I think it is too strict. Moreover, it will be difficult to relax the restriction once this warning is added. On the other hand, we are allowed to have some false negatives for some rare use cases and make warnings for them later. So I think I will remove this warning. @rapidsna @patrykstefanski WDYT?

Thanks for bringing this up!

I disagree that it's easier to add warnings later than relaxing later, since relaxing the rules doesn't disrupt existing adopters while new warnings do. But that said, if there's no reason for preferring one cast over the other it seems reasonable to remove it.

Where's the test case to allow C-Style casts?

rapidsna · 2025-02-20T00:18:29Z

clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp


+#ifndef INTEROP_MODE


Looks like the code isn't actually shared for INTEROP_MODE vs !INTEROP_MODE. Shouldn't it be just a separate test file?

sure. These are things left un-upstreamed.

rapidsna · 2025-02-20T00:46:18Z

clang/lib/Sema/SemaCast.cpp

@@ -238,6 +238,17 @@ namespace {
          const auto *DstPointerType = DestType->getAs<ValueTerminatedType>();
          IsDstNullTerm =
              DstPointerType->getTerminatorValue(Self.Context).isZero();
+          if (Self.LangOpts.isBoundsSafetyAttributeOnlyMode() &&
+              Self.LangOpts.CPlusPlus &&
+              !Self.Diags.isIgnored(


Why is it controlled by diag::warn_unsafe_assign_to_null_terminated_pointer, not by the UnsafeBufferUsage?

Usually, we use diag::warn_unsafe_buffer_operation to indicate whether -Wunsafe-buffer-usage is on for a source location. (For example: here.)

The diag::warn_unsafe_assign_to_null_terminated_pointer is under the UnsafeBufferUsage warning group and is only controlled by the -Wunsafe-buffer-usage flag. So functionally it is equivalent to using diag::warn_unsafe_buffer_operation. I originally thought it is more specific but now I feel like using diag::warn_unsafe_buffer_operation is more consistent and probably more readable. I will change it to diag::warn_unsafe_buffer_operation, if this is your concern.

Yes, consistently using diag::warn_unsafe_buffer_operation sounds a more robust implementation rather than relying on different warnings for each individual set of warnings.

Nit: You might also want to have a helper function like IsSafeBufferMode and use that instead to avoid mistakes and duplication.

rapidsna · 2025-02-20T00:47:07Z

clang/lib/Sema/SemaCast.cpp

+            // In C++ Safe Buffers/Bounds Safety interop mode, the compiler
+            // reports a warning under -Wunsafe-buffer-usage:
+            DiagKind = diag::warn_unsafe_assign_to_null_terminated_pointer;
+            isInvalid = true;


Shouldn't it be isInvalid = false because this is a warning. Then, it's controlled by -Werror if the compiler still wants to fail the build.

yes, it is a mistake!

rapidsna · 2025-02-20T00:48:02Z

clang/lib/Sema/SemaCast.cpp

+    if (DestType->getAs<ValueTerminatedType>()) {
+      Diag(E->getBeginLoc(),
+           diag::warn_unsafe_named_cast_to_null_terminated_pointer);
+      return ExprError();


Similarly, it's weird that it returns ExprError() on a warning. It should either an error or just pass through.

I will remove it.

rapidsna · 2025-02-20T00:48:44Z

clang/lib/Sema/SemaCast.cpp

@@ -576,6 +587,17 @@ Sema::BuildCXXNamedCast(SourceLocation OpLoc, tok::TokenKind Kind,

  Op.checkQualifiedDestType();

+  // In C++ Safe Buffers/Bounds Safety interop mode, forbidding  for now all
+  // named cast to __null_terminated pointer type.
+  if (DestType->isPointerType() && LangOpts.hasBoundsSafetyAttributes() &&


Did you mean Self.LangOpts.isBoundsSafetyAttributeOnlyMode()?

My general idea is that the interoperation work should be guarded by LangOpts.hasBoundsSafetyAttributes combined with the testing of whether-Wunsafe-buffer-usage is enabled for most places. We only use LangOpts.isBoundsSafetyAttributeOnlyMode() to guard against the work that otherwise can affect -fbounds-safety. Let me know if this is not the best way of thinking.

LangOpts.hasBoundsSafetyAttributes is enabled in both the annotation only mode and the full firebloom mode. I was asking because you may want an error and may be a different diagnostics in the firebloom mode, which we didn't design yet. The current way of doing it is still fine for this patch and we can adjust it later for C++ firebloom mode.

rapidsna · 2025-02-20T00:51:47Z

clang/lib/Sema/SemaCast.cpp

+  // In C++ Safe Buffers/Bounds Safety interop mode, forbidding  for now all
+  // named cast to __null_terminated pointer type.
+  if (DestType->isPointerType() && LangOpts.hasBoundsSafetyAttributes() &&
+      !Diags.isIgnored(diag::warn_unsafe_named_cast_to_null_terminated_pointer,


Again, why it's guarded by this specific diag, not the unsafe buffer usage flag?

clang/lib/Sema/SemaExpr.cpp

rapidsna · 2025-02-20T01:42:11Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -13603,6 +13603,19 @@ def note_unsafe_count_attributed_pointer_argument : Note<
 def warn_unsafe_single_pointer_argument : Warning<
  "unsafe assignment to function parameter of __single pointer type">,
  InGroup<UnsafeBufferUsage>, DefaultIgnore;
+def warn_unsafe_assign_to_null_terminated_pointer : Warning<


This is probably almost a duplicate of an existing one warn_bounds_safety_incompatible_non_terminated_by_to_terminated_by. I guess you introduced a new one because you want something in the UsafeBufferUsage group? Okay, I see the diagnostic message at the end is different.

warn_bounds_safety_incompatible_non_terminated_by_to_terminated_by mentions "indexable" and "unsafe_forge", which do not exist in the interop mode. This is the main reason for having another diagnostic message.

You could move those "indexable" and "unsafe_forge" to %select in warn_bounds_safety_incompatible_non_terminated_by_to_terminated_by and not print them in interop.

… of irrelevance.

- Make the diagnostic message simply saying "unsafe [conversion operation] of '__null_terminated' type"; further explanation is put in notes. - Use `warn_unsafe_buffer_operation` to consistently test if CXX Safe Buffer is enabled. Add an helper function for the test. - Remove the warning on using C++ named cast to obtain __null_terminated pointers. - Added tests for good cases of C-Style casts. - Consistently use isBoundsSafetyAttributeOnlyMode for code involving bounds attributes but not shared with full Bounds Safety mode.

patrykstefanski · 2025-02-20T23:50:01Z

clang/lib/Sema/SemaCast.cpp

@@ -205,7 +205,7 @@ namespace {
      /// int *__bidi_indexable p1;
      /// int *__indexable p2 = (int *__indexable)p1;
      /// \endcode
-      if (Self.getLangOpts().BoundsSafety) {
+      if (Self.getLangOpts().hasBoundsSafetyAttributes()) {


Nit: We want to remove hasBoundsSafetyAttributes(), since it is not needed anymore. Self.getLangOpts().hasBoundsSafetyAttributes() -> Self.getLangOpts().BoundsSafetyAttributes.

patrykstefanski · 2025-02-20T23:57:37Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -13603,6 +13603,19 @@ def note_unsafe_count_attributed_pointer_argument : Note<
 def warn_unsafe_single_pointer_argument : Warning<
  "unsafe assignment to function parameter of __single pointer type">,
  InGroup<UnsafeBufferUsage>, DefaultIgnore;
+def warn_unsafe_assign_to_null_terminated_pointer : Warning<


You could move those "indexable" and "unsafe_forge" to %select in warn_bounds_safety_incompatible_non_terminated_by_to_terminated_by and not print them in interop.

patrykstefanski · 2025-02-21T00:00:47Z

clang/include/clang/Sema/Sema.h

@@ -1560,6 +1560,11 @@ class Sema final : public SemaBase {
  /// be controlled with \#pragma clang abi_ptr_attr set(*ATTR*). Defaults to
  /// __single in user code and __unsafe_indexable in system headers.
  BoundsSafetyPointerAttributes CurPointerAbi;
+
+  /// \returns true iff `-Wunsafe-buffer-usage` is enabled for `Loc`.
+  bool isCXXSafeBuffersEnabledAt(SourceLocation Loc) {


Nit: Can this method be const?

patrykstefanski · 2025-02-21T00:06:37Z

clang/lib/Sema/SemaExpr.cpp

+              // match fails:
+              ObjTypeII = RecordDecl->getIdentifier();
+            if (MethodDeclII && ObjTypeII &&
+                MethodDeclII->getName() == "c_str" &&


If there a reason we support std::string::c_str() but not std::string::data()?

std::string::data() is not null-termination guaranteed as I remember.

https://en.cppreference.com/w/cpp/string/basic_string/c_str
c_str() and data() perform the same function. (since C++11)

Aha, it's changed since C++11.

patrykstefanski · 2025-02-21T00:07:27Z

clang/lib/Sema/SemaExpr.cpp

+            if (MethodDeclII && ObjTypeII &&
+                MethodDeclII->getName() == "c_str" &&
+                ObjTypeII->getName() == "basic_string")
+              return Sema::Compatible;


Does this work if LHS is terminated by a value different than 0, e.g. const char *__terminated_by(42)?

Good point. Need to check it is 0-terminator.

patrykstefanski · 2025-02-21T00:17:10Z

clang/lib/Sema/SemaCast.cpp

+            DiagKind = diag::warn_unsafe_assign_to_null_terminated_pointer;
+            isInvalid = false;
+            break;
+          }


Same here, we don't check if the terminator is 0.

I have changed it to re-use warn_bounds_safety_incompatible_non_terminated_by_to_terminated_by. We do not need to check terminator there now.

- Change LangOpts.hasBoundsSafetyAttributes() to LangOpts.BoundsSafetyAttributes - Correct that std::string::c_str() should be not compatible with (non-0)-terminated pointers

Reuse the existing warn_bounds_safety_incompatible_non_terminated_by_to_terminated_by

ziqingluo-90 · 2025-02-21T02:22:31Z

I have addressed comments by:

Remove irrelevant changes in test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp
Reuse warn_bounds_safety_incompatible_non_terminated_by_to_terminated_by for interop's diagnostic. Hide irrelevant texts in "%select".
Use warn_unsafe_buffer_operation to consistently test if CXX Safe Buffer is enabled.
Remove the warning on __null_terminated casting with C++ named casts.
Add tests for OK cases of C-Style casts.
Consistently use isBoundsSafetyAttributeOnlyMode to guard the code that involves bounds attributes but is not shared with full Bounds Safety mode.
Change LangOpts.hasBoundsSafetyAttributes() to LangOpts.BoundsSafetyAttributes
std::string::c_str/data() should only be compatible with 0-terminated pointers

@patrykstefanski @rapidsna @hnrklssn @dtarditi please take another look

ziqingluo-90 · 2025-02-21T21:55:55Z

@swift-ci test

rapidsna · 2025-02-21T22:41:43Z

clang/lib/Sema/SemaExpr.cpp

@@ -20694,6 +20720,9 @@ bool Sema::DiagnoseAssignmentResult(AssignConvertType ConvTy,
      AssignedVarFixIts;
  VarDecl *BoundsSafetyIncompatNestedPtrLocalVarToFix = nullptr;
  FixItHint BoundsSafetyIncompatNestedPtrFixIt;
+  bool InCXXBoundsSafetyMode = LangOpts.CPlusPlus &&


Nit: This seems to be duplicated too in another Sema file. So it's another candidate to get a helper function that tales Loc.

hnrklssn · 2025-02-22T01:21:48Z

clang/lib/Sema/SemaCast.cpp

      /// BoundsSafety: Such case should be handled as BoundsSafetyPointerCast.
      /// \code
      /// int *__bidi_indexable p1;
      /// int *__indexable p2 = (int *__indexable)p1;
      /// \endcode
-      if (Self.getLangOpts().BoundsSafety) {
+      if (Self.getLangOpts().BoundsSafetyAttributes) {
        unsigned DiagKind = 0;
        bool isInvalid = false;
        // The type error may be nested, so any pointer can result in VT errors


Do we want to assert that Self.getLangOpts().BoundsSafety || ConvertTy == Sema::Compatible || (ConvertTy == Sema::IncompatibleNonValueTerminatedToValueTerminatedPointer && InCXXBoundsSafetyMode)? Especially since CheckValueTerminatedAssignmentConstraints used to only run under BoundsSafety, but now it runs under BoundsSafetyAttributes even for C and in C++ without safe buffers mode. We really don't want it to accidentally start returning errors in these modes. Otherwise LGTM

Fair point. But maybe the following is enough?
Self.getLangOpts().BoundsSafety || Self.isCXXSafeBuffersBoundsSafetyInteropEnabledAt(SrcExpr.get()->getBeginLoc()) ,
where isCXXSafeBuffersBoundsSafetyInteropEnabledAt is defined by

bool isCXXSafeBuffersBoundsSafetyInteropEnabledAt(SourceLocation Loc) const { return LangOpts.CPlusPlus && LangOpts.isBoundsSafetyAttributeOnlyMode() && isCXXSafeBuffersEnabledAt(Loc); }

That works for me 👍

- Refactor interop mode check to a helper function - Guard the type checking for C-style cast so that it runs only in full bounds safety or interop only mode

…)` in SemaInit

ziqingluo-90 · 2025-02-22T02:30:15Z

I plan to merge this PR this weekend. @hnrklssn @rapidsna

…e interop mode (swiftlang#10060) For a __null_terminated pointer 'p', only a __null_terminated pointer, a string literal or a call to `std::string::c_str/data` can be assigned to (/passed to/used to initialize, etc.) 'p'. Otherwise, an error will be emitted. The implementation reuses Bounds Safety's diagnostics. (rdar://138798346)

…e interop mode (#10060) (#10103) For a __null_terminated pointer 'p', only a __null_terminated pointer, a string literal or a call to `std::string::c_str/data` can be assigned to (/passed to/used to initialize, etc.) 'p'. Otherwise, an error will be emitted. The implementation reuses Bounds Safety's diagnostics. (rdar://138798346)

ziqingluo-90 requested review from malavikasamak, t-rasmud, hnrklssn, rapidsna and patrykstefanski February 19, 2025 01:11

ziqingluo-90 force-pushed the dev/ziqing/PR-138798346 branch from 0be9793 to 6a7f6c7 Compare February 19, 2025 01:49

ziqingluo-90 force-pushed the dev/ziqing/PR-138798346 branch from 6a7f6c7 to 1e578f1 Compare February 19, 2025 19:25

dtarditi reviewed Feb 19, 2025

View reviewed changes

hnrklssn reviewed Feb 19, 2025

View reviewed changes

rapidsna reviewed Feb 20, 2025

View reviewed changes

ziqingluo-90 added 2 commits February 20, 2025 11:47

Remove changes in warn-unsafe-buffer-usage-libc-functions.cpp because…

e8e5a60

… of irrelevance.

ziqingluo-90 force-pushed the dev/ziqing/PR-138798346 branch from 17a8a92 to 453073f Compare February 21, 2025 00:02

patrykstefanski reviewed Feb 21, 2025

View reviewed changes

ziqingluo-90 added 3 commits February 20, 2025 16:26

Address more comments.

eebc20c

- Change LangOpts.hasBoundsSafetyAttributes() to LangOpts.BoundsSafetyAttributes - Correct that std::string::c_str() should be not compatible with (non-0)-terminated pointers

Remove warn_unsafe_assign_to_null_terminated_pointer

1607947

Reuse the existing warn_bounds_safety_incompatible_non_terminated_by_to_terminated_by

Add that std::string::data() is also null-terminated

7f1b823

ziqingluo-90 mentioned this pull request Feb 21, 2025

[-Wunsafe-buffer-usage] Accept calls to some libc functions with annotated arguments #10088

Merged

patrykstefanski approved these changes Feb 21, 2025

View reviewed changes

rapidsna reviewed Feb 21, 2025

View reviewed changes

hnrklssn reviewed Feb 22, 2025

View reviewed changes

ziqingluo-90 added 2 commits February 21, 2025 18:15

Address minor comments

623da8f

- Refactor interop mode check to a helper function - Guard the type checking for C-style cast so that it runs only in full bounds safety or interop only mode

Use `BoundsSafety || isCXXSafeBuffersBoundsSafetyInteropEnabledAt(...…

92d269a

…)` in SemaInit

ziqingluo-90 merged commit fe73f37 into swiftlang:next Feb 23, 2025

ziqingluo-90 deleted the dev/ziqing/PR-138798346 branch February 25, 2025 19:33


		#ifndef INTEROP_MODE

[-Wunsafe-buffer-usage][BoundsSafety] support __null_terminated in the interop mode #10060

[-Wunsafe-buffer-usage][BoundsSafety] support __null_terminated in the interop mode #10060

Uh oh!

Conversation

ziqingluo-90 commented Feb 19, 2025

Uh oh!

ziqingluo-90 commented Feb 19, 2025

Uh oh!

ziqingluo-90 commented Feb 19, 2025

Uh oh!

ziqingluo-90 commented Feb 19, 2025

Uh oh!

ziqingluo-90 commented Feb 19, 2025

Uh oh!

dtarditi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziqingluo-90 Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziqingluo-90 Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rapidsna Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziqingluo-90 Feb 20, 2025 •

edited

Loading

ziqingluo-90 Feb 20, 2025 •

edited

Loading

rapidsna Feb 20, 2025 •

edited

Loading

ziqingluo-90 Feb 21, 2025 •

edited

Loading

ziqingluo-90 commented Feb 21, 2025 •

edited

Loading

rapidsna Feb 21, 2025 •

edited

Loading