[analyzer][StdLibraryFunctionsChecker] Remove strcasecmp

[martong]

Summary

There are 2 reasons to remove strcasecmp and strncasecmp.

1. They are also modeled in CStringChecker and the related argumentum contraints are checked there.
2. The argument constraints are checked in CStringChecker::evalCall. This is fundamentally flawed, they should be checked in checkPreCall. Even if we set up CStringChecker as a weak dependency for StdLibraryFunctionsChecker then the latter reports the warning always. Besides, CStringChecker fails to discover the constraint violation before the call, so, its evalCall returns with true and then StdCLibraryFunctions also tries to evaluate, this causes an assertion in CheckerManager.

Either we fix CStringChecker to handle the call prerequisites in
checkPreCall, or we must not evaluate any pure functions in
StdCLibraryFunctions that are also handled in CStringChecker.
We do the latter in this patch.

[steakhal]

I completely agree with you.
I plan to further refactor the CStringChecker, but the related patches are pretty much stuck :D

I think this workaround is fine for now.
You might as well extend the corresponding parts of the CStringChecker to make the modelling more precise.
It shouldn't be much of a hassle.
What do you say about that?

clang/test/Analysis/std-c-library-functions-arg-cstring-dependency.c

13

I'm not sure if this is required.

[martong]

In D87239#2259345, @steakhal wrote:

I completely agree with you.
I plan to further refactor the CStringChecker, but the related patches are pretty much stuck :D

I think this workaround is fine for now.
You might as well extend the corresponding parts of the CStringChecker to make the modelling more precise.
It shouldn't be much of a hassle.
What do you say about that?

I think the modeling is well done and precise. I mean, it seems like all of the constraints that I am removing here are handled in CStringChecker. It checks the pointer arguments whether they are null. Also, the length is checked in case of strncasecmp, here:

if (IsBounded) {
  // Get the max number of characters to compare.
  const Expr *lenExpr = CE->getArg(2);
  SVal lenVal = state->getSVal(lenExpr, LCtx);

  // If the length is known, we can get the right substrings.
  if (const llvm::APSInt *len = svalBuilder.getKnownValue(state, lenVal)) {
    // Create substrings of each to compare the prefix.
    LeftStrRef = LeftStrRef.substr(0, (size_t)len->getZExtValue());
    RightStrRef = RightStrRef.substr(0, (size_t)len->getZExtValue());
    canComputeResult = true;
  }
} else { .... }

So, the problem is rather that the constraint check should be done in checkPreCall, but that should be in an NFC refactoring.

[martong]

@steakhal Ping.

I completely agree with you.

So, how should we proceed? :)

[steakhal]

In D87239#2259428, @martong wrote:

So, the problem is rather that the constraint check should be done in checkPreCall, but that should be in an NFC refactoring.

You are right, but we can not fix everything at once. There are quite a few changes under review related to the CStringChecker already.

In D87239#2263634, @martong wrote:

I completely agree with you.

So, how should we proceed? :)

I'm fine with taking this reponsibilty out from the StdLibraryFunctionsChecker.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

addToFunctionSummaryMap("seekdir", Summary(ArgTypes{DirPtrTy, LongTy},
                                                RetType{VoidTy}, NoEvalCall)
                                            .ArgConstraint(NotNull(ArgNo(0))));
 
     // int rand_r(unsigned int *seedp);
     addToFunctionSummaryMap("rand_r", Summary(ArgTypes{UnsignedIntPtrTy},
                                               RetType{IntTy}, NoEvalCall)
                                           .ArgConstraint(NotNull(ArgNo(0))));
 
-    // int strcasecmp(const char *s1, const char *s2);
-    addToFunctionSummaryMap("strcasecmp",
-                            Summary(ArgTypes{ConstCharPtrTy, ConstCharPtrTy},
-                                    RetType{IntTy}, EvalCallAsPure)
-                                .ArgConstraint(NotNull(ArgNo(0)))
-                                .ArgConstraint(NotNull(ArgNo(1))));
-
-    // int strncasecmp(const char *s1, const char *s2, size_t n);
-    addToFunctionSummaryMap(
-        "strncasecmp", Summary(ArgTypes{ConstCharPtrTy, ConstCharPtrTy, SizeTy},
-                               RetType{IntTy}, EvalCallAsPure)
-                           .ArgConstraint(NotNull(ArgNo(0)))
-                           .ArgConstraint(NotNull(ArgNo(1)))
-                           .ArgConstraint(ArgumentCondition(
-                               2, WithinRange, Range(0, SizeMax))));
-
     // int fileno(FILE *stream);
     addToFunctionSummaryMap(
         "fileno", Summary(ArgTypes{FilePtrTy}, RetType{IntTy}, NoEvalCall)
                       .ArgConstraint(NotNull(ArgNo(0))));
 
     // int fseeko(FILE *stream, off_t offset, int whence);
     addToFunctionSummaryMap(
         "fseeko",

clang/test/Analysis/std-c-library-functions-POSIX.c

 // CHECK: Loaded summary for: int pclose(FILE *stream)
 // CHECK: Loaded summary for: int close(int fildes)
 // CHECK: Loaded summary for: long fpathconf(int fildes, int name)
 // CHECK: Loaded summary for: long pathconf(const char *path, int name)
 // CHECK: Loaded summary for: FILE *fdopen(int fd, const char *mode)
 // CHECK: Loaded summary for: void rewinddir(DIR *dir)
 // CHECK: Loaded summary for: void seekdir(DIR *dirp, long loc)
 // CHECK: Loaded summary for: int rand_r(unsigned int *seedp)
-// CHECK: Loaded summary for: int strcasecmp(const char *s1, const char *s2)
-// CHECK: Loaded summary for: int strncasecmp(const char *s1, const char *s2, size_t n)
 // CHECK: Loaded summary for: int fileno(FILE *stream)
 // CHECK: Loaded summary for: int fseeko(FILE *stream, off_t offset, int whence)
 // CHECK: Loaded summary for: off_t ftello(FILE *stream)
 // CHECK: Loaded summary for: void *mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset)
 // CHECK: Loaded summary for: void *mmap64(void *addr, size_t length, int prot, int flags, int fd, off64_t offset)
 // CHECK: Loaded summary for: int pipe(int fildes[2])
 // CHECK: Loaded summary for: off_t lseek(int fildes, off_t offset, int whence)
 // CHECK: Loaded summary for: ssize_t readlink(const char *restrict path, char *restrict buf, size_t bufsize)

 int pclose(FILE *stream);
 int close(int fildes);
 long fpathconf(int fildes, int name);
 long pathconf(const char *path, int name);
 FILE *fdopen(int fd, const char *mode);
 void rewinddir(DIR *dir);
 void seekdir(DIR *dirp, long loc);
 int rand_r(unsigned int *seedp);
-int strcasecmp(const char *s1, const char *s2);
-int strncasecmp(const char *s1, const char *s2, size_t n);
 int fileno(FILE *stream);
 int fseeko(FILE *stream, off_t offset, int whence);
 off_t ftello(FILE *stream);
 void *mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset);
 typedef off_t off64_t;
 void *mmap64(void *addr, size_t length, int prot, int flags, int fd, off64_t offset);
 int pipe(int fildes[2]);
 off_t lseek(int fildes, off_t offset, int whence);

clang/test/Analysis/std-c-library-functions-arg-cstring-dependency.c

+// This test case crashes if strncasecmp is modeled in StdCLibraryFunctions.
+// Either we fix CStringChecker to handle the call prerequisites in
+// checkPreCall, or we must not evaluate any pure functions in
+// StdCLibraryFunctions that are also handled in CStringChecker.
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=unix.cstring.NullArg \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=alpha.unix.StdCLibraryFunctionArgs \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+typedef __typeof(sizeof(int)) size_t;
+int strncasecmp(const char *s1, const char *s2, size_t n);
+
+int strncasecmp_null_argument(char *a, size_t n) {
+  char *b = 0;
+  return strncasecmp(a, b, n); // expected-warning{{Null pointer passed as 2nd argument to string comparison function}}
+}