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

ziqingluo-90 · 2025-02-21T19:08:52Z

printf, fprintf snprintf functions accept __null_terminated
snprintf function accepts __counted_by/__sized_by
functions consuming a single string pointer like strlen or atoi accept __null_terminated

Generalized isCountAttributedPointerArgumentSafe so that it is shared by interoperation gadgets and the unsafe libc gadget.

(A follow-up change to rdar://138798346)

ziqingluo-90 · 2025-02-21T19:09:39Z

CC @dtarditi

ziqingluo-90 · 2025-02-21T19:10:24Z

This PR depends on #10060

ziqingluo-90 · 2025-02-21T21:57:58Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -747,7 +747,7 @@ const Expr *extractExtentFromSubviewDataCall(ASTContext &Context,
 static bool hasIntegeralConstant(const Expr *E, uint64_t Val, ASTContext &Ctx) {
  Expr::EvalResult ER;

-  if (E->EvaluateAsConstantExpr(ER, Ctx)) {
+  if (E->EvaluateAsInt(ER, Ctx)) {


According to https://github.com/llvm/llvm-project/pull/124022/files, the result of EvaluateAsConstantExpr may not necessarily be able to getInt() later.

ziqingluo-90 · 2025-02-21T22:01:07Z

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

@@ -23,7 +23,7 @@ namespace std {
    T* p;
    T *c_str();
    T *data();
-    unsigned size_bytes();


A mistake I made in the test---std::string has no size_bytes method.

It's probably worth checking if basic_string<wchar_t> works as expected.

✅ Added test cases for wide characters.

ziqingluo-90 · 2025-02-22T00:24:14Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -366,6 +366,7 @@ isInUnspecifiedUntypedContext(internal::Matcher<Stmt> InnerMatcher) {

 namespace {

+/* TO_UPSTREAM(BoundsSafetyInterop) ON */


I assume there is no problem in upstreaming the part of the interop in UnsafeBufferUsage.cpp? @patrykstefanski

Yup, this is fine.

ziqingluo-90 · 2025-02-24T19:54:43Z

add @jkorous-apple

patrykstefanski · 2025-02-24T22:13:54Z

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

+  snwprintf(safe_p, n, "%s", str); // expected-warning{{function 'snwprintf' is unsafe}} expected-note{{string argument is not guaranteed to be null-terminated}}
+
+  memcpy(safe_p, safe_p, n);               // no warn
+  strlen(str);                             // expected-warning{{unsafe assignment to a parameter of '__null_terminated' type; only '__null_terminated' pointers, string literals, and 'std::string::c_str' calls are compatible with '__null_terminated' pointers}}


I'm not getting this warning here.

error: 'expected-warning' diagnostics expected but not seen: File ./clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions-interop.cpp Line 36: unsafe assignment to a parameter of '__null_terminated' type; only '__null_terminated' pointers, string literals, and 'std::string::c_str' calls are compatible with '__null_terminated' pointers

✅ It was not up to date to the dependent PR. Fixed.

patrykstefanski · 2025-02-24T23:50:47Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

-                                   &*ValuesOpt, Context);
+  return isCountAttributedPointerArgumentSafeImpl(
+      Context, Arg, CountArg, CAT, CAT->getCountExpr(), CAT->isCountInBytes(),
+      CAT->isOrNull(), ValuesOpt ? &*ValuesOpt : nullptr);


Nit: IMHO we could be explicit to avoid any confusion: ValuesOpt.has_value() ? &*ValuesOpt : nullptr.

patrykstefanski · 2025-02-25T00:02:40Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+  assert(CountedByExpr ||
+         !DependentValueMap &&
+             "If the __counted_by information is hardcoded, there is no "
+             "dependent value map.");


Is the operator precedence in those 2 asserts correct? The bottom one is equivalent to:

CountedByExpr || (!DependentValueMap && "If the __counted_by information is hardcoded, there is no " "dependent value map.")

Is this expected?

The string literal is simply true so I think they are equivalent. But let's add parentheses to avoid confusion.

patrykstefanski · 2025-02-25T00:12:06Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

-  auto ValuesOpt = getDependentValuesFromCall(CAT, Call);
-  if (!ValuesOpt.has_value())
+  if (!DependentValueMap && CountedByExpr)
+    // Bail if the map is not available for case (a). Becuase


Becuase -> Because.

patrykstefanski · 2025-02-25T00:12:22Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

    return false;

-  const Expr *ArgCount = nullptr;
+  // the acutal count of the pointer inferred through patterns below:


acutal -> actual

patrykstefanski · 2025-02-25T00:44:36Z

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

+
+// expected-note@+2{{consider using a safe container and passing '.data()' to the parameter 'dst' and '.size()' to its dependent parameter 'size' or 'std::span' and passing '.first(...).data()' to the parameter 'dst'}}
+// expected-note@+1{{consider using a safe container and passing '.data()' to the parameter 'src' and '.size()' to its dependent parameter 'size' or 'std::span' and passing '.first(...).data()' to the parameter 'src'}}
+void memcpy(void * __sized_by(size) dst, const void * __sized_by(size) src, unsigned size);


memcpy returns void * and takes size_t size.

patrykstefanski · 2025-02-25T00:45:55Z

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

+void memcpy(void * __sized_by(size) dst, const void * __sized_by(size) src, unsigned size);
+unsigned strlen( const char* __null_terminated str );
+// expected-note@+1{{consider using a safe container and passing '.data()' to the parameter 'buffer' and '.size()' to its dependent parameter 'buf_size' or 'std::span' and passing '.first(...).data()' to the parameter 'buffer'}}
+int snprintf( char* __counted_by(buf_size) buffer, unsigned buf_size, const char* format, ... );


Nit: Those printf functions take size_t.

patrykstefanski · 2025-02-25T00:47:30Z

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

+// expected-note@+1{{consider using a safe container and passing '.data()' to the parameter 'buffer' and '.size()' to its dependent parameter 'buf_size' or 'std::span' and passing '.first(...).data()' to the parameter 'buffer'}}
+int snprintf( char* __counted_by(buf_size) buffer, unsigned buf_size, const char* format, ... );
+int snwprintf( char* __counted_by(buf_size) buffer, unsigned buf_size, const char* format, ... );
+int vsnprintf( char* __counted_by(buf_size) buffer, unsigned buf_size, const char* format, ... );


Should take va_list vlist instead of ....

hmm... then I need to define a va_list that accepts variant parameters.

Could you elaborate? Is calling vsnprintf expected to be always unsafe? Cannot we declare this function with va_list and then pass an empty va_list on the call site?

ok, that would work.

patrykstefanski · 2025-02-25T01:12:11Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

-  }
-  return true; // ptr and size are not in safe pattern
+  return !isHardcodedCountedByPointerArgumentSafe(
+      Finder->getASTContext(), Buf, Size, FirstParmTy.getTypePtr(), true,


Is true passed to isSizedBy correct here? I think the functions taking wchar_t would be annotated with __counted_by().

that's a good point!

patrykstefanski · 2025-02-25T01:14:56Z

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

@@ -23,7 +23,7 @@ namespace std {
    T* p;
    T *c_str();
    T *data();
-    unsigned size_bytes();


It's probably worth checking if basic_string<wchar_t> works as expected.

…tated arguments - `printf`, `fprintf` `snprintf` functions accept `__null_terminated` - `snprintf` function accepts `__counted_by/__sized_by` - functions consuming a single string pointer like `strlen` or `atoi` accept `__null_terminated` Generalized `isCountAttributedPointerArgumentSafe` so that it is shared by interoperation gadgets and the unsafe libc gadget. (A follow-up change to rdar://138798346)

ziqingluo-90 · 2025-02-26T01:11:51Z

@patrykstefanski I have addressed your comments. Plz take another look.

ziqingluo-90 · 2025-02-26T01:12:25Z

Also added the pattern---std::span{p, strlen(p)}

patrykstefanski · 2025-02-26T23:15:19Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+    } else
+      // When there is no argument representing the count/size, it is safe iff
+      // the annotation is `__counted_by(1)`.
+      return !isSizedBy && hasIntegeralConstant(CountedByExpr, 1, Context);


If we have CountArg above, we support form 2 (&var, sizeof(var)) with this check:

// form 2: if (auto TySize = Context.getTypeSizeInCharsIfKnown(DRE->getType())) return hasIntegeralConstant(CountArg, TySize->getQuantity(), Context);

In the else branch here, we don't check for the form 2, we only check form 1. Is this expected?

patrykstefanski · 2025-02-26T23:15:38Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+    } else
+      // When there is no argument representing the count/size, it is safe iff
+      // the annotation is `__counted_by(1)`.
+      return !isSizedBy && hasIntegeralConstant(CountedByExpr, 1, Context);
    return false;
  }


Yup, that's fine.

patrykstefanski · 2025-02-26T23:30:11Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+      return (FName == "strlen" || FName == "wcslen") &&
+             AreSameDRE(Arg0, CallArg);
+    }
+  }


Is the check for __null_terminated missing?

size_t strlen( const char* __null_terminated str ); size_t wcslen( const wchar_t* __null_terminated str ); void test_span_ctor_non_nt(char *p, const wchar_t *wp) { std::span S{p, strlen(p)}; std::span S2{wp, wcslen(wp)}; }

It looks like we don't warn for this code. Not sure why it doesn't warn for the call strlen(p) though.

hmm... yeah, I'll look into it.

ok, it's because that test only has -Wunsafe-buffer-usage-in-container but no -Wunsafe-buffer-usage. This is expected.

I have added a test for this.

patrykstefanski · 2025-02-26T23:33:26Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -933,7 +1014,7 @@ bool isSinglePointerArgumentSafe(ASTContext &Context, const Expr *Arg) {
 //   6. `std::span<T>{p, n}`, where `p` is a __counted_by(`n`)/__sized_by(`n`)
 //   pointer OR `std::span<char>{(char*)p, n}`, where `p` is a __sized_by(`n`)
 //   pointer.
-
+//   7. `std::span<char>{p, strlen(p)}` or `std::span<wchar_t>{p, wcslen(p)}`


I would add '... if p is __null_terminated`.

Actually, whether p is __null_terminated is irrelevant here.
The analysis of std::span constructors only cares about if the two arguments are in the form p and strlen(p).
If p is not null-terminated, it's up to the type checking to complain about it. Just like we do not check that p is of pointer-to-char type here.

patrykstefanski · 2025-02-26T23:46:16Z

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

+    T *c_str();
+    T *data();
+    unsigned size();
+  };


Can we match the interface to be the same as the types in std::? You can copy-paste from warn-unsafe-buffer-usage-count-attributed-pointer-argument.cpp.

patrykstefanski · 2025-02-26T23:54:38Z

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

+// expected-note@+1{{consider using a safe container and passing '.data()' to the parameter 'buffer' and '.size()' to its dependent parameter 'buf_size' or 'std::span' and passing '.first(...).data()' to the parameter 'buffer'}}
+int snprintf( char* __counted_by(buf_size) buffer, unsigned buf_size, const char* format, ... );
+int snwprintf( char* __counted_by(buf_size) buffer, unsigned buf_size, const char* format, ... );
+int vsnprintf( char* __counted_by(buf_size) buffer, unsigned buf_size, const char* format, ... );


Could you elaborate? Is calling vsnprintf expected to be always unsafe? Cannot we declare this function with va_list and then pass an empty va_list on the call site?

patrykstefanski · 2025-02-27T00:01:11Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

-      }
-    }
+  if (auto NumChars =
+          Finder->getASTContext().getTypeSizeInCharsIfKnown(FirstPteTy)) {


Nit: auto and explicit optional:

std::optional<CharUnits> NumChars = Finder->getASTContext().getTypeSizeInCharsIfKnown(FirstPteTy); if (NumChars.has_value()) {

isn't if (std::optional<CharUnits> NumChars = ...) better?

patrykstefanski · 2025-03-03T23:48:26Z

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

+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -Wno-error=bounds-safety-strict-terminated-by-cast\
+// RUN:            -verify -fexperimental-bounds-safety-attributes %s
+#include <ptrcheck.h>
+typedef unsigned size_t;


Cannot we include stddef.h? unsigned is not right type on most 64-bit platforms.

…tated arguments (#10088) * [-Wunsafe-buffer-usage] Accept calls to some libc functions with annotated arguments - `printf`, `fprintf` `snprintf` functions accept `__null_terminated` - `snprintf` function accepts `__counted_by/__sized_by` - functions consuming a single string pointer like `strlen` or `atoi` accept `__null_terminated` Generalized `isCountAttributedPointerArgumentSafe` so that it is shared by interoperation gadgets and the unsafe libc gadget. (A follow-up change to rdar://138798346)

…tated arguments & Let libc warnings yield to bounds attributes (#10182) Cherry-pick two commits: * [Upstream][C++ Safe Buffers] Let libc warnings yield to bounds attributes For a call to an unsafe libc function, do not warn about it if its callee function has any bounds attributes. Because we can verify the safety of the call using bounds attributes. (rdar://140138380) * [-Wunsafe-buffer-usage] Accept calls to some libc functions with annotated arguments (#10088) - `printf`, `fprintf` `snprintf` functions accept `__null_terminated` - `snprintf` function accepts `__counted_by/__sized_by` - functions consuming a single string pointer like `strlen` or `atoi` accept `__null_terminated` Generalized `isCountAttributedPointerArgumentSafe` so that it is shared by interoperation gadgets and the unsafe libc gadget. (A follow-up change to rdar://138798346)

ziqingluo-90 requested review from malavikasamak and patrykstefanski February 21, 2025 19:08

ziqingluo-90 commented Feb 21, 2025

View reviewed changes

ziqingluo-90 commented Feb 22, 2025

View reviewed changes

ziqingluo-90 requested review from t-rasmud, hnrklssn and rapidsna February 24, 2025 19:54

patrykstefanski reviewed Feb 25, 2025

View reviewed changes

ziqingluo-90 added 3 commits February 25, 2025 11:33

Mark code in UnsafeBufferUsage.cpp that needs upstream

a33e41a

Address comments

2678bd8

ziqingluo-90 force-pushed the dev/ziqing/PR-138798346-followup branch from 306ea22 to 2678bd8 Compare February 26, 2025 00:45

add comments for sprintf test

db61bca

patrykstefanski reviewed Feb 27, 2025

View reviewed changes

ziqingluo-90 added 2 commits February 28, 2025 11:01

address comments

4cce6a8

Improve the fidelity of vsnprintf declarations in a test

bad0c19

patrykstefanski reviewed Mar 3, 2025

View reviewed changes

add stddef.h to a test

ddce071

patrykstefanski approved these changes Mar 4, 2025

View reviewed changes

ziqingluo-90 merged commit cfe50ed into swiftlang:next Mar 5, 2025

ziqingluo-90 deleted the dev/ziqing/PR-138798346-followup branch March 5, 2025 01:55

ziqingluo-90 mentioned this pull request Mar 6, 2025

[🍒][-Wunsafe-buffer-usage] Accept calls to some libc functions with annotated arguments & Let libc warnings yield to bounds attributes #10182

Merged

		@@ -366,6 +366,7 @@ isInUnspecifiedUntypedContext(internal::Matcher<Stmt> InnerMatcher) {

		namespace {

		/* TO_UPSTREAM(BoundsSafetyInterop) ON */

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

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

Conversation

ziqingluo-90 commented Feb 21, 2025

ziqingluo-90 commented Feb 21, 2025

ziqingluo-90 commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziqingluo-90 commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziqingluo-90 commented Feb 26, 2025

ziqingluo-90 commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment