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

[NoQ]

Summary

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).

[NoQ]

test/Analysis/string-with-signedness.c

1 ↗

(On Diff #120834)

We do have a warning for this.

[xazax.hun]

This patch looks good to me.
I am wondering whether it would make sense to have some kind of warning (even in the frontend if not in the analyzer) for using standard functions with non-standard signatures, so users start to file bugs for the maintainers of those libraries.

[xazax.hun]

test/Analysis/string-with-signedness.c

1 ↗

(On Diff #120834)

Oh, so we already have a warning for this. Good :)

cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C,
   const MemRegion *R = l.getAsRegion();
   if (!R)
     return state;
 
   const ElementRegion *ER = dyn_cast<ElementRegion>(R);
   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());
   SValBuilder &svalBuilder = C.getSValBuilder();
   SVal Extent =
     svalBuilder.convertToArrayIndex(superReg->getExtent(svalBuilder));
   DefinedOrUnknownSVal Size = Extent.castAs<DefinedOrUnknownSVal>();
 
bool CStringChecker::IsFirstBufInBound(CheckerContext &C,
   const MemRegion *R = BufEnd.getAsRegion();
   if (!R)
     return true; // cf top comment.
 
   const ElementRegion *ER = dyn_cast<ElementRegion>(R);
   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");
 
   // Get the size of the array.
   const SubRegion *superReg = cast<SubRegion>(ER->getSuperRegion());
   SVal Extent =
       svalBuilder.convertToArrayIndex(superReg->getExtent(svalBuilder));
   DefinedOrUnknownSVal ExtentSize = Extent.castAs<DefinedOrUnknownSVal>();

cfe/trunk/test/Analysis/string-with-signedness.c

+// 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);
+}