[PATCH] D39422: [analyzer] pr34779: CStringChecker: Don't get crashed by non-standard standard library function definitions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.

An assertion failure was triggered in pr34779 by defining `strcpy` as `void (unsigned char *, unsigned char *)`, while normally it is `char *(char *, char *)`. The assertion is removed because normally we try not to crash in such cases, but simply refuse to model. For now i did not try to actually model the modified strcpy, because it requires more work (unhardcode `CharTy` in a lot of places around the checker), and supporting other aspects of the non-standard definition requires even more work (i.e. we try to bind the return value, need to disable this mechanism when the return type is void), and if we try to model other similar functions (are other string functions accepting unsigned chars as well?) it'd bloat quite a bit.

So //for now the analyzer would not find C string errors// in code that defines string functions in a non-standard manner. It simply tries to avoid crashing. To think - maybe we should add more defensive checks (eg. into `CallDescription`).


https://reviews.llvm.org/D39422

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/string-with-signedness.c


Index: test/Analysis/string-with-signedness.c
===================================================================
--- /dev/null
+++ test/Analysis/string-with-signedness.c
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 -Wno-incompatible-library-redeclaration -analyzer-checker=core,unix.cstring,alpha.unix.cstring -verify %s
+
+// expected-no-diagnostics
+
+void *strcpy(unsigned char *, unsigned char *);
+
+unsigned char a, b;
+void testUnsignedStrcpy() {
+  strcpy(&a, &b);
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -289,8 +289,8 @@
   if (!ER)
     return state;
 
-  assert(ER->getValueType() == C.getASTContext().CharTy &&
-    "CheckLocation should only be called with char* ElementRegions");
+  if (ER->getValueType() != C.getASTContext().CharTy)
+    return state;
 
   // Get the size of the array.
   const SubRegion *superReg = cast<SubRegion>(ER->getSuperRegion());
@@ -874,6 +874,8 @@
   if (!ER)
     return true; // cf top comment.
 
+  // FIXME: Does this crash when a non-standard definition
+  // of a library function is encountered?
   assert(ER->getValueType() == C.getASTContext().CharTy &&
          "IsFirstBufInBound should only be called with char* ElementRegions");
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D39422.120834.patch
Type: text/x-patch
Size: 1381 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171030/b4fae7db/attachment-0001.bin>