[Clang] Added clang diagnostic when snprintf/vsnprintf uses sizeof(de…

[BgZun]

…st) for the len parameter

Closes: #162366

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (BgZun)

Changes

…st) for the len parameter

Closes: #162366

---

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

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4)
• (modified) clang/lib/Sema/SemaChecking.cpp (+42-31)
• (modified) clang/test/SemaCXX/warn-memset-bad-sizeof.cpp (+9)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 51f07256c5d9f..ee15880ce92ab 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -444,6 +444,9 @@ Improvements to Clang's diagnostics
   comparison operators when mixed with bitwise operators in enum value initializers.
   This can be locally disabled by explicitly casting the initializer value.
 
+- Clang now emits ``-Wsizeof-pointer-memaccess`` when snprintf/vsnprintf use the sizeof 
+  the destination buffer(dynamically allocated) in the len parameter(#GH162366)
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 53aa86a7dabde..a496aee8e0c9e 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -895,6 +895,10 @@ def warn_sizeof_pointer_type_memaccess : Warning<
   "argument to 'sizeof' in %0 call is the same pointer type %1 as the "
   "%select{destination|source}2; expected %3 or an explicit length">,
   InGroup<SizeofPointerMemaccess>;
+def warn_sizeof_pointer_dest_type_memacess : Warning<
+  "argument to 'sizeof' in %0 call is the same expression as the "
+  "destination; did you mean to put an explicit length?">,
+  InGroup<SizeofPointerMemaccess>;
 def warn_strlcpycat_wrong_size : Warning<
   "size argument in %0 call appears to be size of the source; "
   "expected the size of the destination">,
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index f4e58de91286b..00e0fc8521fa6 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -1139,6 +1139,37 @@ static bool ProcessFormatStringLiteral(const Expr *FormatExpr,
   return false;
 }
 
+static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
+  if (const auto *Unary = dyn_cast<UnaryExprOrTypeTraitExpr>(E))
+    if (Unary->getKind() == UETT_SizeOf)
+      return Unary;
+  return nullptr;
+}
+
+/// If E is a sizeof expression, returns its argument expression,
+/// otherwise returns NULL.
+static const Expr *getSizeOfExprArg(const Expr *E) {
+  if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
+    if (!SizeOf->isArgumentType())
+      return SizeOf->getArgumentExpr()->IgnoreParenImpCasts();
+  return nullptr;
+}
+
+/// If E is a sizeof expression, returns its argument type.
+static QualType getSizeOfArgType(const Expr *E) {
+  if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
+    return SizeOf->getTypeOfArgument();
+  return QualType();
+}
+
+/// Check if two expressions refer to the same declaration.
+static bool referToTheSameDecl(const Expr *E1, const Expr *E2) {
+  if (const DeclRefExpr *D1 = dyn_cast_or_null<DeclRefExpr>(E1))
+    if (const DeclRefExpr *D2 = dyn_cast_or_null<DeclRefExpr>(E2))
+      return D1->getDecl() == D2->getDecl();
+  return false;
+}
+
 void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
                                                CallExpr *TheCall) {
   if (TheCall->isValueDependent() || TheCall->isTypeDependent() ||
@@ -1449,6 +1480,17 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
       }
     }
     DestinationSize = ComputeSizeArgument(0);
+    const Expr *SizeOfArg = TheCall->getArg(1)->IgnoreParenImpCasts();
+    const Expr *Dest = TheCall->getArg(0)->IgnoreParenImpCasts();
+    const Expr *SizeOfArgExpr = getSizeOfExprArg(SizeOfArg);
+    const QualType SizeOfArgType = getSizeOfArgType(SizeOfArg);
+    const Type *ExprType = SizeOfArgType.getTypePtrOrNull();
+    if (ExprType && ExprType->isPointerType() &&
+        referToTheSameDecl(SizeOfArgExpr, Dest)) {
+      DiagRuntimeBehavior(SizeOfArg->getExprLoc(), Dest,
+                          PDiag(diag::warn_sizeof_pointer_dest_type_memacess)
+                              << FD->getNameInfo().getName());
+    }
   }
   }
 
@@ -9979,29 +10021,6 @@ static const CXXRecordDecl *getContainedDynamicClass(QualType T,
   return nullptr;
 }
 
-static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
-  if (const auto *Unary = dyn_cast<UnaryExprOrTypeTraitExpr>(E))
-    if (Unary->getKind() == UETT_SizeOf)
-      return Unary;
-  return nullptr;
-}
-
-/// If E is a sizeof expression, returns its argument expression,
-/// otherwise returns NULL.
-static const Expr *getSizeOfExprArg(const Expr *E) {
-  if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
-    if (!SizeOf->isArgumentType())
-      return SizeOf->getArgumentExpr()->IgnoreParenImpCasts();
-  return nullptr;
-}
-
-/// If E is a sizeof expression, returns its argument type.
-static QualType getSizeOfArgType(const Expr *E) {
-  if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
-    return SizeOf->getTypeOfArgument();
-  return QualType();
-}
-
 namespace {
 
 struct SearchNonTrivialToInitializeField
@@ -10499,14 +10518,6 @@ void Sema::CheckStrlcpycatArguments(const CallExpr *Call,
                                       OS.str());
 }
 
-/// Check if two expressions refer to the same declaration.
-static bool referToTheSameDecl(const Expr *E1, const Expr *E2) {
-  if (const DeclRefExpr *D1 = dyn_cast_or_null<DeclRefExpr>(E1))
-    if (const DeclRefExpr *D2 = dyn_cast_or_null<DeclRefExpr>(E2))
-      return D1->getDecl() == D2->getDecl();
-  return false;
-}
-
 static const Expr *getStrlenExprArg(const Expr *E) {
   if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
     const FunctionDecl *FD = CE->getDirectCallee();
diff --git a/clang/test/SemaCXX/warn-memset-bad-sizeof.cpp b/clang/test/SemaCXX/warn-memset-bad-sizeof.cpp
index 0a78caa924ea9..bacb2c167af14 100644
--- a/clang/test/SemaCXX/warn-memset-bad-sizeof.cpp
+++ b/clang/test/SemaCXX/warn-memset-bad-sizeof.cpp
@@ -188,3 +188,12 @@ void strcpy_and_friends() {
   strndup(FOO, sizeof(FOO)); // \
       // expected-warning {{'strndup' call operates on objects of type 'const char' while the size is based on a different type 'const char *'}} expected-note{{did you mean to provide an explicit length?}}
 }
+
+extern "C" int snprintf(char* buffer, __SIZE_TYPE__ buf_size, const char* format, ...);
+extern "C" void* malloc(unsigned size);
+
+void check_prints(){
+    char* a = (char*) malloc(20);
+    const char* b = "Hello World";
+    snprintf(a, sizeof(a), "%s", b); // expected-warning{{argument to 'sizeof' in 'snprintf' call is the same expression as the destination; did you mean to put an explicit length?}}
+}

[ojhunt]

I think a better solution here would to be to pull the logic that is diagnosing the same mistake in the memory accessing functions into a separate reusable function - DiagnoseSuspiciousBufferSizeof or something (my naming skills are terrible).

See the logic in Sema::CheckMemaccessArguments where we gate the diagnostics on warn_sizeof_pointer_expr_memaccess being enabled - that's what I think we should be investigating lifting into a separate function that can be used in other similarly behaving functions, either completely or partially.

[ojhunt]

We should avoid this large code movement, we could forward declare these functions instead, but I think a better approach be to move the actual diagnosis logic into a separate function.

[ojhunt]

You should reuse warn_sizeof_pointer_type_memaccess and the associated note.

[ojhunt]

Rather than a reimplementation of similar semantics we should consider refactoring the existing checks so they can be used here instead.