[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

TBH we don't really have processes there. And its a bit of a mess, regarding 
CSA.  Usually we forgot to update the release notes, and at best we aggregate 
the changes just prior a release for the release notes. But we haven't done it 
for the last two releases for sure.

I was thinking of asking you, but given this context we wouldn't gain much. We 
would still need to go through the diffs anyway. So, let not touch the notes 
now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner added a comment.

@steakhal should the release notes page be updated to add this?

I think this could be beneficial for the users to be aware of that change.

If yes, who is responsible for doing that? Developers? (me) or someone else is 
taking care of reviewing the list of changes since latest releases?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner added a comment.

Thanks @steakhal for the proposal :) but I pushed it myself: 
https://github.com/llvm/llvm-project/commit/ce97312d109b21acb97d3ea243e214f20bd87cfc

I used to have svn access, and Chris kindly give me write access to the git 
repository.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGce97312d109b: Implement BufferOverlap check for 
sprint/snprintf (authored by ArnaudBienner).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/buffer-overlap.c

Index: clang/test/Analysis/buffer-overlap.c
===
--- /dev/null
+++ clang/test/Analysis/buffer-overlap.c
@@ -0,0 +1,98 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+
+// This provides us with four possible sprintf() definitions.
+
+#ifdef USE_BUILTINS
+#define BUILTIN(f) __builtin_##f
+#else /* USE_BUILTINS */
+#define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+typedef typeof(sizeof(int)) size_t;
+
+#ifdef VARIANT
+
+#define __sprintf_chk BUILTIN(__sprintf_chk)
+#define __snprintf_chk BUILTIN(__snprintf_chk)
+int __sprintf_chk (char * __restrict str, int flag, size_t os,
+const char * __restrict fmt, ...);
+int __snprintf_chk (char * __restrict str, size_t len, int flag, size_t os,
+const char * __restrict fmt, ...);
+
+#define sprintf(str, ...) __sprintf_chk(str, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+#define snprintf(str, len, ...) __snprintf_chk(str, len, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+
+#else /* VARIANT */
+
+#define sprintf BUILTIN(sprintf)
+int sprintf(char *restrict buffer, const char *restrict format, ... );
+int snprintf(char *restrict buffer, size_t bufsz,
+ const char *restrict format, ... );
+#endif /* VARIANT */
+
+void test_sprintf1() {
+  char a[4] = {0};
+  sprintf(a, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf4() {
+  char a[4] = {0};
+  sprintf(a, "%d", 42); // no-warning
+}
+
+void test_sprintf5() {
+  char a[4] = {0};
+  char b[4] = {0};
+  sprintf(a, "%s", b); // no-warning
+}
+
+void test_snprintf1() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf2() {
+  char a[4] = {0};
+  snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf5() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d", 42); // no-warning
+}
+
+void test_snprintf6() {
+  char a[4] = {0};
+  char b[4] = {0};
+  snprintf(a, sizeof(a), "%s", b); // no-warning
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -12,6 +12,7 @@
 //===--===//
 
 #include "InterCheckerAPI.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
@@ -175,6 +176,8 @@
std::bind(::evalMemcmp, _1, _2, _3, CK_Regular)},
   {{CDF_MaybeBuiltin, {"bzero"}, 2}, ::evalBzero},
   {{CDF_MaybeBuiltin, {"explicit_bzero"}, 2}, ::evalBzero},
+  {{CDF_MaybeBuiltin, {"sprintf"}, 2}, ::evalSprintf},
+  {{CDF_MaybeBuiltin, {"snprintf"}, 2}, ::evalSnprintf},
   };
 
   // These require a bit of special handling.
@@ -228,6 +231,11 @@
   void evalMemset(CheckerContext , const CallExpr *CE) const;
   void evalBzero(CheckerContext , const CallExpr *CE) const;
 
+  void evalSprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSnprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSprintfCommon(CheckerContext , const CallExpr *CE, bool IsBounded,
+ bool IsBuiltin) const;
+
   // Utility methods
   std::pair
   static 

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner updated this revision to Diff 526743.
ArnaudBienner added a comment.

- clang-format fix


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/buffer-overlap.c

Index: clang/test/Analysis/buffer-overlap.c
===
--- /dev/null
+++ clang/test/Analysis/buffer-overlap.c
@@ -0,0 +1,98 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+
+// This provides us with four possible sprintf() definitions.
+
+#ifdef USE_BUILTINS
+#define BUILTIN(f) __builtin_##f
+#else /* USE_BUILTINS */
+#define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+typedef typeof(sizeof(int)) size_t;
+
+#ifdef VARIANT
+
+#define __sprintf_chk BUILTIN(__sprintf_chk)
+#define __snprintf_chk BUILTIN(__snprintf_chk)
+int __sprintf_chk (char * __restrict str, int flag, size_t os,
+const char * __restrict fmt, ...);
+int __snprintf_chk (char * __restrict str, size_t len, int flag, size_t os,
+const char * __restrict fmt, ...);
+
+#define sprintf(str, ...) __sprintf_chk(str, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+#define snprintf(str, len, ...) __snprintf_chk(str, len, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+
+#else /* VARIANT */
+
+#define sprintf BUILTIN(sprintf)
+int sprintf(char *restrict buffer, const char *restrict format, ... );
+int snprintf(char *restrict buffer, size_t bufsz,
+ const char *restrict format, ... );
+#endif /* VARIANT */
+
+void test_sprintf1() {
+  char a[4] = {0};
+  sprintf(a, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf4() {
+  char a[4] = {0};
+  sprintf(a, "%d", 42); // no-warning
+}
+
+void test_sprintf5() {
+  char a[4] = {0};
+  char b[4] = {0};
+  sprintf(a, "%s", b); // no-warning
+}
+
+void test_snprintf1() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf2() {
+  char a[4] = {0};
+  snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf5() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d", 42); // no-warning
+}
+
+void test_snprintf6() {
+  char a[4] = {0};
+  char b[4] = {0};
+  snprintf(a, sizeof(a), "%s", b); // no-warning
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -12,6 +12,7 @@
 //===--===//
 
 #include "InterCheckerAPI.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
@@ -175,6 +176,8 @@
std::bind(::evalMemcmp, _1, _2, _3, CK_Regular)},
   {{CDF_MaybeBuiltin, {"bzero"}, 2}, ::evalBzero},
   {{CDF_MaybeBuiltin, {"explicit_bzero"}, 2}, ::evalBzero},
+  {{CDF_MaybeBuiltin, {"sprintf"}, 2}, ::evalSprintf},
+  {{CDF_MaybeBuiltin, {"snprintf"}, 2}, ::evalSnprintf},
   };
 
   // These require a bit of special handling.
@@ -228,6 +231,11 @@
   void evalMemset(CheckerContext , const CallExpr *CE) const;
   void evalBzero(CheckerContext , const CallExpr *CE) const;
 
+  void evalSprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSnprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSprintfCommon(CheckerContext , const CallExpr *CE, bool IsBounded,
+ bool IsBuiltin) const;
+
   // Utility methods
   std::pair
   static assumeZero(CheckerContext ,
@@ -2352,6 +2360,51 @@
   C.addTransition(State);
 }
 
+void 

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Balázs Benics via Phabricator via cfe-commits]

steakhal accepted this revision.
steakhal added a comment.

Can you commit this change or should I do it on your behalf?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner updated this revision to Diff 526463.
ArnaudBienner added a comment.

- Code review comments: change back the string buffer check + run clang format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/buffer-overlap.c

Index: clang/test/Analysis/buffer-overlap.c
===
--- /dev/null
+++ clang/test/Analysis/buffer-overlap.c
@@ -0,0 +1,98 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+
+// This provides us with four possible sprintf() definitions.
+
+#ifdef USE_BUILTINS
+#define BUILTIN(f) __builtin_##f
+#else /* USE_BUILTINS */
+#define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+typedef typeof(sizeof(int)) size_t;
+
+#ifdef VARIANT
+
+#define __sprintf_chk BUILTIN(__sprintf_chk)
+#define __snprintf_chk BUILTIN(__snprintf_chk)
+int __sprintf_chk (char * __restrict str, int flag, size_t os,
+const char * __restrict fmt, ...);
+int __snprintf_chk (char * __restrict str, size_t len, int flag, size_t os,
+const char * __restrict fmt, ...);
+
+#define sprintf(str, ...) __sprintf_chk(str, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+#define snprintf(str, len, ...) __snprintf_chk(str, len, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+
+#else /* VARIANT */
+
+#define sprintf BUILTIN(sprintf)
+int sprintf(char *restrict buffer, const char *restrict format, ... );
+int snprintf(char *restrict buffer, size_t bufsz,
+ const char *restrict format, ... );
+#endif /* VARIANT */
+
+void test_sprintf1() {
+  char a[4] = {0};
+  sprintf(a, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf4() {
+  char a[4] = {0};
+  sprintf(a, "%d", 42); // no-warning
+}
+
+void test_sprintf5() {
+  char a[4] = {0};
+  char b[4] = {0};
+  sprintf(a, "%s", b); // no-warning
+}
+
+void test_snprintf1() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf2() {
+  char a[4] = {0};
+  snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf5() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d", 42); // no-warning
+}
+
+void test_snprintf6() {
+  char a[4] = {0};
+  char b[4] = {0};
+  snprintf(a, sizeof(a), "%s", b); // no-warning
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -12,6 +12,7 @@
 //===--===//
 
 #include "InterCheckerAPI.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
@@ -175,6 +176,8 @@
std::bind(::evalMemcmp, _1, _2, _3, CK_Regular)},
   {{CDF_MaybeBuiltin, {"bzero"}, 2}, ::evalBzero},
   {{CDF_MaybeBuiltin, {"explicit_bzero"}, 2}, ::evalBzero},
+  {{CDF_MaybeBuiltin, {"sprintf"}, 2}, ::evalSprintf},
+  {{CDF_MaybeBuiltin, {"snprintf"}, 2}, ::evalSnprintf},
   };
 
   // These require a bit of special handling.
@@ -228,6 +231,11 @@
   void evalMemset(CheckerContext , const CallExpr *CE) const;
   void evalBzero(CheckerContext , const CallExpr *CE) const;
 
+  void evalSprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSnprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSprintfCommon(CheckerContext , const CallExpr *CE, bool IsBounded,
+ bool IsBuiltin) const;
+
   // Utility methods
   std::pair
   static assumeZero(CheckerContext ,

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Ah, I can see that pre-merge bots failed due to clang-format violations.
Please make all the touched lines as clang-formatted prior to committing 
anything.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Balázs Benics via Phabricator via cfe-commits]

steakhal accepted this revision.
steakhal added a comment.
This revision is now accepted and ready to land.

Yea, it still looks okay. Let me know if you don't have commit access. In that 
case, also let me know what should be used for the commit author.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner added a comment.

@steakhal did you get a chance to look at my comment?

I would really love to see this merged upstream if you think this could be a 
beneficial change :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner added a comment.

Giving this a second thought, I feel like the initial check:

  if (!ArgExpr->getType()->isAnyPointerType() ||
  !ArgExpr->getType()->getPointeeType()->isAnyCharacterType())

is better than the new one. To me it reads like "expr type is a pointer and it 
points to character type" which is more understandable IMHO.

If you're worried about the expression being a bit long, I could move type to a 
temp variable:

  if (const QualType type = ArgExpr->getType();
  !type->isAnyPointerType() ||
  !type->getPointeeType()->isAnyCharacterType())

Though I'm not sure this is really more readable.

What do you think?

Any other suggestion/comment about this patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D150430#4347365 , @ArnaudBienner 
wrote:

> Thanks for the review :)
>
> I implemented the suggested changes.
>
> Just one question: `PointeeTy.isNull()`: is this guaranteed to always work 
> even though `getType()->isAnyPointerType() == false`?
>
> I'm assuming yes since the tests still pass, but wanted to confirm this is 
> the best way to do this check for `char*` arguments.

I was also surprised, but check the definition of that function. It will try to 
cast the nonnull type to a list of types, and return null if didnt match.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner added a comment.

Thanks for the review :)

I implemented the suggested changes.

Just one question: `PointeeTy.isNull()`: is this guaranteed to always work even 
though `getType()->isAnyPointerType() == false`?

I'm assuming yes since the tests still pass, but wanted to confirm this is the 
best way to do this check for `char*` arguments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner updated this revision to Diff 522738.
ArnaudBienner added a comment.

- Code review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/buffer-overlap.c

Index: clang/test/Analysis/buffer-overlap.c
===
--- /dev/null
+++ clang/test/Analysis/buffer-overlap.c
@@ -0,0 +1,98 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+
+// This provides us with four possible sprintf() definitions.
+
+#ifdef USE_BUILTINS
+#define BUILTIN(f) __builtin_##f
+#else /* USE_BUILTINS */
+#define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+typedef typeof(sizeof(int)) size_t;
+
+#ifdef VARIANT
+
+#define __sprintf_chk BUILTIN(__sprintf_chk)
+#define __snprintf_chk BUILTIN(__snprintf_chk)
+int __sprintf_chk (char * __restrict str, int flag, size_t os,
+const char * __restrict fmt, ...);
+int __snprintf_chk (char * __restrict str, size_t len, int flag, size_t os,
+const char * __restrict fmt, ...);
+
+#define sprintf(str, ...) __sprintf_chk(str, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+#define snprintf(str, len, ...) __snprintf_chk(str, len, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+
+#else /* VARIANT */
+
+#define sprintf BUILTIN(sprintf)
+int sprintf(char *restrict buffer, const char *restrict format, ... );
+int snprintf(char *restrict buffer, size_t bufsz,
+ const char *restrict format, ... );
+#endif /* VARIANT */
+
+void test_sprintf1() {
+  char a[4] = {0};
+  sprintf(a, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf4() {
+  char a[4] = {0};
+  sprintf(a, "%d", 42); // no-warning
+}
+
+void test_sprintf5() {
+  char a[4] = {0};
+  char b[4] = {0};
+  sprintf(a, "%s", b); // no-warning
+}
+
+void test_snprintf1() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf2() {
+  char a[4] = {0};
+  snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf5() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d", 42); // no-warning
+}
+
+void test_snprintf6() {
+  char a[4] = {0};
+  char b[4] = {0};
+  snprintf(a, sizeof(a), "%s", b); // no-warning
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -12,6 +12,7 @@
 //===--===//
 
 #include "InterCheckerAPI.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
@@ -175,6 +176,8 @@
std::bind(::evalMemcmp, _1, _2, _3, CK_Regular)},
   {{CDF_MaybeBuiltin, {"bzero"}, 2}, ::evalBzero},
   {{CDF_MaybeBuiltin, {"explicit_bzero"}, 2}, ::evalBzero},
+  {{CDF_MaybeBuiltin, {"sprintf"}, 2}, ::evalSprintf},
+  {{CDF_MaybeBuiltin, {"snprintf"}, 2}, ::evalSnprintf},
   };
 
   // These require a bit of special handling.
@@ -228,6 +231,11 @@
   void evalMemset(CheckerContext , const CallExpr *CE) const;
   void evalBzero(CheckerContext , const CallExpr *CE) const;
 
+  void evalSprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSnprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSprintfCommon(CheckerContext , const CallExpr *CE, bool IsBounded,
+ bool IsBuiltin) const;
+
   // Utility methods
   std::pair
   static assumeZero(CheckerContext ,
@@ -2352,6 +2360,50 @@
   C.addTransition(State);
 }
 

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Here is the migrated version of the mentioned discussion: 
https://discourse.llvm.org/t/static-or-dynamic-code-analysis-for-undefined-behavior-in-sprintf/59624


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I like it.

How about an implementation like this:

  void CStringChecker::evalSprintfCommon(CheckerContext , const CallExpr *CE,
 bool IsBounded) const {
ProgramStateRef State = C.getState();
DestinationArgExpr Dest = {CE->getArg(0), 0};
  
const auto NumParams = CE->getCalleeDecl()->getAsFunction()->getNumParams();
assert(CE->getNumArgs() >= NumParams);
  
auto AllArguments =
llvm::make_range(CE->getArgs(), CE->getArgs() + CE->getNumArgs());
auto VariadicArguments = drop_begin(enumerate(AllArguments), NumParams);
  
for (auto [ArgIdx, ArgExpr] : VariadicArguments) {
  // We consider only string buffers
  if (QualType PointeeTy = ArgExpr->getType()->getPointeeType();
  PointeeTy.isNull() || !PointeeTy->isAnyCharacterType())
continue;
  SourceArgExpr Source = {ArgExpr, unsigned(ArgIdx)};
  
  // Ensure the buffers do not overlap.
  SizeArgExpr SrcExprAsSizeDummy = {Source.Expression, 
Source.ArgumentIndex};
  State = CheckOverlap(
  C, State,
  (IsBounded ? SizeArgExpr{CE->getArg(1), 1} : SrcExprAsSizeDummy), 
Dest,
  Source);
  if (!State)
return;
}
  
C.addTransition(State);
  }



- Using ranges to iterate over the variadic arguments. This way it will just 
work with both builtins and whatnot.
- Adding a transition to preserve the gained knowledge about the relation of 
the source and destination buffers. In this case, it won't be really effective, 
but at least we obey this convention.

Maybe add a few test cases where the warning should not be present.
And it is also valuable to have at least one test case asserting the complete 
message.




Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:179-180
   {{CDF_MaybeBuiltin, {"explicit_bzero"}, 2}, ::evalBzero},
+  {{CDF_MaybeBuiltin, {"sprintf"}}, ::evalSprintf},
+  {{CDF_MaybeBuiltin, {"snprintf"}}, ::evalSnprintf},
   };

Let's express that we expect at least 2 arguments at the callsites.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner updated this revision to Diff 522020.
ArnaudBienner added a comment.

Updating D150430 : Implement BufferOverlap 
check for sprint/snprintf


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/buffer-overlap.c

Index: clang/test/Analysis/buffer-overlap.c
===
--- /dev/null
+++ clang/test/Analysis/buffer-overlap.c
@@ -0,0 +1,76 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+
+// This provides us with four possible sprintf() definitions.
+
+#ifdef USE_BUILTINS
+#define BUILTIN(f) __builtin_##f
+#else /* USE_BUILTINS */
+#define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+typedef typeof(sizeof(int)) size_t;
+
+#ifdef VARIANT
+
+#define __sprintf_chk BUILTIN(__sprintf_chk)
+#define __snprintf_chk BUILTIN(__snprintf_chk)
+int __sprintf_chk (char * __restrict str, int flag, size_t os,
+const char * __restrict fmt, ...);
+int __snprintf_chk (char * __restrict str, size_t len, int flag, size_t os,
+const char * __restrict fmt, ...);
+
+#define sprintf(str, ...) __sprintf_chk(str, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+#define snprintf(str, len, ...) __snprintf_chk(str, len, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+
+#else /* VARIANT */
+
+#define sprintf BUILTIN(sprintf)
+int sprintf(char *restrict buffer, const char *restrict format, ... );
+int snprintf(char *restrict buffer, size_t bufsz,
+ const char *restrict format, ... );
+#endif /* VARIANT */
+
+void test_sprintf1() {
+  char a[4] = {0};
+  sprintf(a, "%d/%s", 1, a); // expected-warning{{overlapping}}
+}
+
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf1() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf2() {
+  char a[4] = {0};
+  snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{overlapping}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -12,6 +12,7 @@
 //===--===//
 
 #include "InterCheckerAPI.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
@@ -175,6 +176,8 @@
std::bind(::evalMemcmp, _1, _2, _3, CK_Regular)},
   {{CDF_MaybeBuiltin, {"bzero"}, 2}, ::evalBzero},
   {{CDF_MaybeBuiltin, {"explicit_bzero"}, 2}, ::evalBzero},
+  {{CDF_MaybeBuiltin, {"sprintf"}}, ::evalSprintf},
+  {{CDF_MaybeBuiltin, {"snprintf"}}, ::evalSnprintf},
   };
 
   // These require a bit of special handling.
@@ -228,6 +231,11 @@
   void evalMemset(CheckerContext , const CallExpr *CE) const;
   void evalBzero(CheckerContext , const CallExpr *CE) const;
 
+  void evalSprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSnprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSprintfCommon(CheckerContext , const CallExpr *CE, bool IsBounded,
+ bool IsBuiltin) const;
+
   // Utility methods
   std::pair
   static assumeZero(CheckerContext ,
@@ -2352,6 +2360,61 @@
   C.addTransition(State);
 }
 
+void CStringChecker::evalSprintf(CheckerContext , const CallExpr *CE) const {
+  CurrentFunctionDescription = "'sprintf'";
+  bool IsBI = CE->getBuiltinCallee() == Builtin::BI__builtin___sprintf_chk;
+  evalSprintfCommon(C, CE, /* IsBounded */ false, IsBI);
+}
+
+void CStringChecker::evalSnprintf(CheckerContext , const CallExpr *CE) const {
+  CurrentFunctionDescription = "'snprintf'";
+  bool IsBI = CE->getBuiltinCallee() == Builtin::BI__builtin___snprintf_chk;
+  evalSprintfCommon(C, CE, /* IsBounded */ true, IsBI);
+}
+
+void 

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner updated this revision to Diff 522019.
ArnaudBienner added a comment.

Updating D150430 : Implement BufferOverlap 
check for sprint/snprintf


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/buffer-overlap.c


Index: clang/test/Analysis/buffer-overlap.c
===
--- clang/test/Analysis/buffer-overlap.c
+++ clang/test/Analysis/buffer-overlap.c
@@ -45,6 +45,16 @@
   sprintf(a, "%d/%s", 1, a); // expected-warning{{overlapping}}
 }
 
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{overlapping}}
+}
+
 void test_snprintf1() {
   char a[4] = {0};
   snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{overlapping}}
@@ -54,3 +64,13 @@
   char a[4] = {0};
   snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{overlapping}}
 }
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{overlapping}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -2380,9 +2380,9 @@
   // Check that the source and destination do not overlap.
   // Iterate over CE->getNumArgs(), skipping all parameters which are not 
format
   // arguments
-  // For sprintf case, it starts at index 3:
+  // For sprintf case, it starts at position 3/index 2:
   // sprintf(char *buffer, const char* format, ... /* format arguments */);
-  unsigned int format_arguments_start_idx = 3;
+  unsigned int format_arguments_start_idx = 2;
   // snprintf case: one extra extra arguments for size
   // int snprintf(char *buffer, size_t bufsz, const char *format,
   //  ... /* format arguments */);


Index: clang/test/Analysis/buffer-overlap.c
===
--- clang/test/Analysis/buffer-overlap.c
+++ clang/test/Analysis/buffer-overlap.c
@@ -45,6 +45,16 @@
   sprintf(a, "%d/%s", 1, a); // expected-warning{{overlapping}}
 }
 
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{overlapping}}
+}
+
 void test_snprintf1() {
   char a[4] = {0};
   snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{overlapping}}
@@ -54,3 +64,13 @@
   char a[4] = {0};
   snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{overlapping}}
 }
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{overlapping}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -2380,9 +2380,9 @@
   // Check that the source and destination do not overlap.
   // Iterate over CE->getNumArgs(), skipping all parameters which are not format
   // arguments
-  // For sprintf case, it starts at index 3:
+  // For sprintf case, it starts at position 3/index 2:
   // sprintf(char *buffer, const char* format, ... /* format arguments */);
-  unsigned int format_arguments_start_idx = 3;
+  unsigned int format_arguments_start_idx = 2;
   // snprintf case: one extra extra arguments for size
   // int snprintf(char *buffer, size_t bufsz, const char *format,
   //  ... /* format arguments */);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner updated this revision to Diff 522018.
ArnaudBienner added a comment.

Fix start index for sprintf ovlerap check + tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/buffer-overlap.c


Index: clang/test/Analysis/buffer-overlap.c
===
--- clang/test/Analysis/buffer-overlap.c
+++ clang/test/Analysis/buffer-overlap.c
@@ -45,6 +45,16 @@
   sprintf(a, "%d/%s", 1, a); // expected-warning{{overlapping}}
 }
 
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{overlapping}}
+}
+
 void test_snprintf1() {
   char a[4] = {0};
   snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{overlapping}}
@@ -54,3 +64,13 @@
   char a[4] = {0};
   snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{overlapping}}
 }
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{overlapping}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -2380,9 +2380,9 @@
   // Check that the source and destination do not overlap.
   // Iterate over CE->getNumArgs(), skipping all parameters which are not 
format
   // arguments
-  // For sprintf case, it starts at index 3:
+  // For sprintf case, it starts at position 3/index 2:
   // sprintf(char *buffer, const char* format, ... /* format arguments */);
-  unsigned int format_arguments_start_idx = 3;
+  unsigned int format_arguments_start_idx = 2;
   // snprintf case: one extra extra arguments for size
   // int snprintf(char *buffer, size_t bufsz, const char *format,
   //  ... /* format arguments */);


Index: clang/test/Analysis/buffer-overlap.c
===
--- clang/test/Analysis/buffer-overlap.c
+++ clang/test/Analysis/buffer-overlap.c
@@ -45,6 +45,16 @@
   sprintf(a, "%d/%s", 1, a); // expected-warning{{overlapping}}
 }
 
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{overlapping}}
+}
+
 void test_snprintf1() {
   char a[4] = {0};
   snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{overlapping}}
@@ -54,3 +64,13 @@
   char a[4] = {0};
   snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{overlapping}}
 }
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{overlapping}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -2380,9 +2380,9 @@
   // Check that the source and destination do not overlap.
   // Iterate over CE->getNumArgs(), skipping all parameters which are not format
   // arguments
-  // For sprintf case, it starts at index 3:
+  // For sprintf case, it starts at position 3/index 2:
   // sprintf(char *buffer, const char* format, ... /* format arguments */);
-  unsigned int format_arguments_start_idx = 3;
+  unsigned int format_arguments_start_idx = 2;
   // snprintf case: one extra extra arguments for size
   // int snprintf(char *buffer, size_t bufsz, const char *format,
   //  ... /* format arguments */);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2385
+  // sprintf(char *buffer, const char* format, ... /* format arguments */);
+  unsigned int format_arguments_start_idx = 3;
+  // snprintf case: one extra extra arguments for size

Just realized this should be 2 (position 3, but index 2).

Will upload a newer version of the patch with this change, and new tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150430/new/

https://reviews.llvm.org/D150430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner created this revision.
Herald added subscribers: steakhal, martong.
Herald added a reviewer: NoQ.
Herald added a project: All.
ArnaudBienner edited the summary of this revision.
ArnaudBienner added a subscriber: dergachev.a.
ArnaudBienner published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

I discussed about this with you @dergachev.a a long time ago on the mailing 
list ("static or dynamic code analysis for undefined behavior in sprintf") and 
finally took the time to implement something :)

Let me know what you think about this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150430

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/buffer-overlap.c

Index: clang/test/Analysis/buffer-overlap.c
===
--- /dev/null
+++ clang/test/Analysis/buffer-overlap.c
@@ -0,0 +1,56 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+
+// This provides us with four possible sprintf() definitions.
+
+#ifdef USE_BUILTINS
+#define BUILTIN(f) __builtin_##f
+#else /* USE_BUILTINS */
+#define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+typedef typeof(sizeof(int)) size_t;
+
+#ifdef VARIANT
+
+#define __sprintf_chk BUILTIN(__sprintf_chk)
+#define __snprintf_chk BUILTIN(__snprintf_chk)
+int __sprintf_chk (char * __restrict str, int flag, size_t os,
+const char * __restrict fmt, ...);
+int __snprintf_chk (char * __restrict str, size_t len, int flag, size_t os,
+const char * __restrict fmt, ...);
+
+#define sprintf(str, ...) __sprintf_chk(str, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+#define snprintf(str, len, ...) __snprintf_chk(str, len, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+
+#else /* VARIANT */
+
+#define sprintf BUILTIN(sprintf)
+int sprintf(char *restrict buffer, const char *restrict format, ... );
+int snprintf(char *restrict buffer, size_t bufsz,
+ const char *restrict format, ... );
+#endif /* VARIANT */
+
+void test_sprintf1() {
+  char a[4] = {0};
+  sprintf(a, "%d/%s", 1, a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf1() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{overlapping}}
+}
+
+void test_snprintf2() {
+  char a[4] = {0};
+  snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{overlapping}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -12,6 +12,7 @@
 //===--===//
 
 #include "InterCheckerAPI.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
@@ -175,6 +176,8 @@
std::bind(::evalMemcmp, _1, _2, _3, CK_Regular)},
   {{CDF_MaybeBuiltin, {"bzero"}, 2}, ::evalBzero},
   {{CDF_MaybeBuiltin, {"explicit_bzero"}, 2}, ::evalBzero},
+  {{CDF_MaybeBuiltin, {"sprintf"}}, ::evalSprintf},
+  {{CDF_MaybeBuiltin, {"snprintf"}}, ::evalSnprintf},
   };
 
   // These require a bit of special handling.
@@ -228,6 +231,11 @@
   void evalMemset(CheckerContext , const CallExpr *CE) const;
   void evalBzero(CheckerContext , const CallExpr *CE) const;
 
+  void evalSprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSnprintf(CheckerContext , const CallExpr *CE) const;
+  void evalSprintfCommon(CheckerContext , const CallExpr *CE, bool IsBounded,
+ bool IsBuiltin) const;
+
   // Utility methods
   std::pair
   static assumeZero(CheckerContext ,
@@ -2352,6 +2360,61 @@
   C.addTransition(State);
 }
 
+void CStringChecker::evalSprintf(CheckerContext , const CallExpr *CE) const {
+  CurrentFunctionDescription = "'sprintf'";
+  bool IsBI = CE->getBuiltinCallee() == Builtin::BI__builtin___sprintf_chk;
+  evalSprintfCommon(C, CE, /* IsBounded */ false, IsBI);
+}
+
+void CStringChecker::evalSnprintf(CheckerContext , const CallExpr *CE) const {
+  CurrentFunctionDescription = "'snprintf'";
+  bool IsBI = CE->getBuiltinCallee() == Builtin::BI__builtin___snprintf_chk;
+  evalSprintfCommon(C, CE, /* IsBounded */ true, IsBI);
+}
+
+void CStringChecker::evalSprintfCommon(CheckerContext , const CallExpr *CE,
+   bool IsBounded, bool