[PATCH] D71155: [analyzer] CERT: StrChecker: 30.c

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 233905.
Charusso marked 6 inline comments as done.
Charusso added a comment.

- Try to conjure the index of the 'ElementRegion'.


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

https://reviews.llvm.org/D71155

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/cert/str30-c-explain.cpp
  clang/test/Analysis/cert/str30-c-notes.cpp
  clang/test/Analysis/cert/str30-c.cpp

Index: clang/test/Analysis/cert/str30-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str30-c.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.30c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR30-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+typedef __typeof__((char *)0 - (char *)0) ptrdiff_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+char *strrchr(const char *str, int c);
+int puts(const char *str);
+
+namespace test_strrchr_bad {
+const char *get_dirname(const char *pathname) {
+  char *slash;
+  slash = strrchr(pathname, '/');
+  if (slash) {
+*slash = '\0'; /* Undefined behavior */
+// expected-warning@-1 {{'slash' is pointing to a constant string}}
+  }
+  return pathname;
+}
+
+int main(void) {
+  puts(get_dirname(__FILE__));
+  return 0;
+}
+} // namespace test_strrchr_bad
+
+namespace test_strrchr_good {
+char *get_dirname(const char *pathname, char *dirname, size_t size) {
+  const char *slash;
+  slash = strrchr(pathname, '/');
+  if (slash) {
+ptrdiff_t slash_idx = slash - pathname;
+if ((size_t)slash_idx < size) {
+  memcpy(dirname, pathname, slash_idx);
+  dirname[slash_idx] = '\0';
+  return dirname;
+}
+  }
+  return 0;
+}
+
+int main(void) {
+  char dirname[260];
+  if (get_dirname(__FILE__, dirname, sizeof(dirname))) {
+puts(dirname);
+  }
+  return 0;
+}
+} // namespace test_strrchr_good
Index: clang/test/Analysis/cert/str30-c-notes.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str30-c-notes.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.30c \
+// RUN:  -analyzer-output=text -verify %s
+
+// See the examples on the page of STR30-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+char *strrchr(const char *str, int c);
+int puts(const char *str);
+
+namespace test_strrchr_bad {
+const char *get_dirname(const char *pathname) {
+  char *slash;
+  slash = strrchr(pathname, '/');
+  // expected-note@-1 {{'strrchr' returns a pointer to the constant 'pathname'}}
+
+  slash = strrchr(slash, '/');
+  // expected-note@-1 {{'strrchr' returns a pointer to the constant 'pathname'}}
+
+  if (slash) {
+// expected-note@-1 {{'slash' is non-null}}
+// expected-note@-2 {{Taking true branch}}
+
+*slash = '\0'; /* Undefined behavior */
+// expected-note@-1 {{'slash' is pointing to a constant string}}
+// expected-warning@-2 {{'slash' is pointing to a constant string}}
+  }
+  return pathname;
+}
+
+int main(void) {
+  puts(get_dirname(__FILE__));
+  // expected-note@-1 {{Calling 'get_dirname'}}
+  return 0;
+}
+} // namespace test_strrchr_bad
Index: clang/test/Analysis/cert/str30-c-explain.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str30-c-explain.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.30c \
+// RUN:  -analyzer-checker=debug.ExprInspection \
+// RUN:  %s 2>&1 | FileCheck %s
+
+// See the examples on the page of STR30-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals
+
+#include "../Inputs/system-header-simulator.h"
+
+char *strrchr(const char *str, int c);
+
+void clang_analyzer_explain(char *);
+void clang_analyzer_dump(char *);
+
+void dump(const char *pathname) {
+  char *slash;
+  slash = strrchr(pathname, '/');
+
+  clang_analyzer_dump(slash);
+  // CHECK: {pathname,conj_$1{long long, LC1, S{{[0-9]+}}, #1},char}
+
+  clang_analyzer_explain(slash);
+  // CHECK: pointer to element of type 'char' with index 'symbol of type 'long long' conjured at statement 'pathname'' of parameter 'pathname'
+}
Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp

[PATCH] D71155: [analyzer] CERT: StrChecker: 30.c

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2110
+if (const auto *SR = MR->getBaseRegion()->getAs()) {
+  State = State->BindExpr(CE, LCtx, SrcV);
+  C.addTransition(State);

Charusso wrote:
> NoQ wrote:
> > Mmm, that's not a correct return value for these functions. These functions 
> > don't simply pass through their first argument.
> Yes, but we need some index here. It requires a `NonLoc`, so I just randomly 
> picked the first index, but I like the idea of an unknown index. Would we 
> like to introduce `UnknownVal` for indices?
Use the correct region but //conjure the index//.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2122
+
+  SVal ConjuredV = SVB.getConjuredHeapSymbolVal(CE, LCtx, C.blockCount());
+  SVal ResultV = loc::MemRegionVal(SVB.getRegionManager().getElementRegion(

Charusso wrote:
> NoQ wrote:
> > Why "heap"?
> Well, a string which length is at least 16 characters long is going to be 
> allocated on the heap. I have to conjure the string here to create its 
> element.
o.o
```lang=c++
void foo() {
  // This string is 20 characters long
  // but it's clearly on the stack.
  char str[] = "12345678901234567890";
  // This one is therefore also on the stack.
  char *ptr = strchr(str, '0');
}
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D71155



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

[PATCH] D71155: [analyzer] CERT: StrChecker: 30.c

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2109
+  if (const MemRegion *MR = SrcV.getAsRegion()) {
+if (const auto *SR = MR->getBaseRegion()->getAs()) {
+  State = State->BindExpr(CE, LCtx, SrcV);

NoQ wrote:
> Why does it matter whether the region is symbolic or not?
Hm, I think you are right, we could omit that if-statement. I wanted to specify 
to reuse conjured symbols.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2110
+if (const auto *SR = MR->getBaseRegion()->getAs()) {
+  State = State->BindExpr(CE, LCtx, SrcV);
+  C.addTransition(State);

NoQ wrote:
> Mmm, that's not a correct return value for these functions. These functions 
> don't simply pass through their first argument.
Yes, but we need some index here. It requires a `NonLoc`, so I just randomly 
picked the first index, but I like the idea of an unknown index. Would we like 
to introduce `UnknownVal` for indices?



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2122
+
+  SVal ConjuredV = SVB.getConjuredHeapSymbolVal(CE, LCtx, C.blockCount());
+  SVal ResultV = loc::MemRegionVal(SVB.getRegionManager().getElementRegion(

NoQ wrote:
> Why "heap"?
Well, a string which length is at least 16 characters long is going to be 
allocated on the heap. I have to conjure the string here to create its element.


Repository:
  rC Clang

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

https://reviews.llvm.org/D71155



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

[PATCH] D71155: [analyzer] CERT: StrChecker: 30.c

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2109
+  if (const MemRegion *MR = SrcV.getAsRegion()) {
+if (const auto *SR = MR->getBaseRegion()->getAs()) {
+  State = State->BindExpr(CE, LCtx, SrcV);

Why does it matter whether the region is symbolic or not?



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2110
+if (const auto *SR = MR->getBaseRegion()->getAs()) {
+  State = State->BindExpr(CE, LCtx, SrcV);
+  C.addTransition(State);

Mmm, that's not a correct return value for these functions. These functions 
don't simply pass through their first argument.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2122
+
+  SVal ConjuredV = SVB.getConjuredHeapSymbolVal(CE, LCtx, C.blockCount());
+  SVal ResultV = loc::MemRegionVal(SVB.getRegionManager().getElementRegion(

Why "heap"?


Repository:
  rC Clang

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

https://reviews.llvm.org/D71155



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

[PATCH] D71155: [analyzer] CERT: StrChecker: 30.c

[Csaba Dabis via Phabricator via cfe-commits]

Charusso created this revision.
Charusso added a reviewer: NoQ.
Charusso added a project: clang.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Charusso added a comment.
Charusso added a parent revision: D71033: [analyzer] CERT: StrChecker: 32.c.

Examples generated by CodeChecker from the `curl` project:
F10986729: str30-c.tar.gz 


This checker is implemented based on the following rule:
https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals

It warns on misusing the following functions: `strpbrk()`, `strchr()`,
`strrchr()`, `strstr()`, `memchr()`.


Repository:
  rC Clang

https://reviews.llvm.org/D71155

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/cert/str30-c-notes.cpp
  clang/test/Analysis/cert/str30-c.cpp

Index: clang/test/Analysis/cert/str30-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str30-c.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.30.c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR30-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+typedef __typeof__((char*)0-(char*)0) ptrdiff_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+char *strrchr(const char *str, int c);
+int puts(const char *str);
+
+namespace test_strrchr_bad {
+const char *get_dirname(const char *pathname) {
+  char *slash;
+  slash = strrchr(pathname, '/');
+  if (slash) {
+*slash = '\0'; /* Undefined behavior */
+// expected-warning@-1 {{'slash' is pointing to a constant string}}
+  }
+  return pathname;
+}
+
+int main(void) {
+  puts(get_dirname(__FILE__));
+  return 0;
+}
+} // namespace test_strrchr_bad
+
+namespace test_strrchr_good {
+char *get_dirname(const char *pathname, char *dirname, size_t size) {
+  const char *slash;
+  slash = strrchr(pathname, '/');
+  if (slash) {
+ptrdiff_t slash_idx = slash - pathname;
+if ((size_t)slash_idx < size) {
+  memcpy(dirname, pathname, slash_idx);
+  dirname[slash_idx] = '\0'; 
+  return dirname;
+}
+  }
+  return 0;
+}
+
+int main(void) {
+  char dirname[260];
+  if (get_dirname(__FILE__, dirname, sizeof(dirname))) {
+puts(dirname);
+  }
+  return 0;
+}
+} // namespace test_strrchr_good
Index: clang/test/Analysis/cert/str30-c-notes.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str30-c-notes.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.30.c \
+// RUN:  -analyzer-output=text -verify %s
+
+// See the examples on the page of STR30-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+char *strrchr(const char *str, int c);
+int puts(const char *str);
+
+namespace test_strrchr_bad {
+const char *get_dirname(const char *pathname) {
+  char *slash;
+  slash = strrchr(pathname, '/');
+  // expected-note@-1 {{'strrchr' returns a pointer to the constant 'pathname'}}
+  
+  slash = strrchr(slash, '/');
+  // expected-note@-1 {{'strrchr' returns a pointer to the constant 'pathname'}}
+
+  if (slash) {
+// expected-note@-1 {{Assuming 'slash' is non-null}}
+// expected-note@-2 {{Taking true branch}}
+
+*slash = '\0'; /* Undefined behavior */
+// expected-note@-1 {{'slash' is pointing to a constant string}}
+// expected-warning@-2 {{'slash' is pointing to a constant string}}
+  }
+  return pathname;
+}
+
+int main(void) {
+  puts(get_dirname(__FILE__));
+  // expected-note@-1 {{Calling 'get_dirname'}}
+  return 0;
+}
+} // namespace test_strrchr_bad
Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
@@ -12,6 +12,9 @@
 //  https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152038
 //
 //  This checker is a base checker which consist of the following checkers:
+//  - '30.c'
+//  https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals
+//
 //  - '31.c'
 //  

[PATCH] D71155: [analyzer] CERT: StrChecker: 30.c

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Examples generated by CodeChecker from the `curl` project:
F10986729: str30-c.tar.gz 


Repository:
  rC Clang

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

https://reviews.llvm.org/D71155



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