Crash in CStringChecker

[Quuxplusone]

Bugzilla Link	PR41812
Status	NEW
Importance	P release blocker
Reported by	Ádám Balogh (adam.balogh@ericsson.com)
Reported on	2019-05-09 04:21:52 -0700
Last modified on	2019-08-09 20:10:54 -0700
Version	trunk
Hardware	All All
CC	adam.balogh@ericsson.com, daniel.krupp@gmail.com, dcoughlin@apple.com, dkszelethus@gmail.com, dmajor@bugmail.cc, llvm-bugs@lists.llvm.org, noqnoqneo@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

Given the following faulty C code (the programmer forgot that taking address of an array is the same as the array itself which is handled as an address):

char dest[255], **dest_p = &dest;
char src[255];
memcmp((const void*) *dest_p, (const void *) src, sizeof(dest));

Analyzing this code with any C-String checker enabled results in an assertion failure:

clang: llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:104: T clang::ento::SVal::castAs() const [with T = clang::ento::DefinedOrUnknownSVal]: Assertion `T::isKind(*this)' failed.
#0 0x00007fa514b947dd llvm::sys::PrintStackTrace(llvm::raw_ostream&) llvm/lib/Support/Unix/Signals.inc:494:0
 #1 0x00007fa514b94870 PrintStackTraceSignalHandler(void*) llvm/lib/Support/Unix/Signals.inc:558:0
 #2 0x00007fa514b92870 llvm::sys::RunSignalHandlers() llvm/lib/Support/Signals.cpp:68:0
 #3 0x00007fa514b94230 SignalHandler(int) llvm/lib/Support/Unix/Signals.inc:357:0
 #4 0x00007fa510428f20 (/lib/x86_64-linux-gnu/libc.so.6+0x3ef20)
 #5 0x00007fa510428e97 raise /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #6 0x00007fa51042a801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0
 #7 0x00007fa51041a39a __assert_fail_base /build/glibc-OTsEL5/glibc-2.27/assert/assert.c:89:0
 #8 0x00007fa51041a412 (/lib/x86_64-linux-gnu/libc.so.6+0x30412)
 #9 0x00007fa503f28a6f clang::ento::DefinedOrUnknownSVal clang::ento::SVal::castAs<clang::ento::DefinedOrUnknownSVal>() const llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:105:0
#10 0x00007fa503f58630 (anonymous namespace)::CStringChecker::evalMemcmp(clang::ento::CheckerContext&, clang::CallExpr const*) const llvm/tools/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1286:0
#11 0x00007fa503f5e5cc (anonymous namespace)::CStringChecker::evalCall(clang::CallExpr const*, clang::ento::CheckerContext&) const llvm/tools/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2354:0

[Quuxplusone]

I think this happens because you don't have core checkers enabled. If you enable core checkers, the uninitialized value checker will emit a fatal error and prevent the undefined value from reaching CStringChecker. There are a lot of places in the code in which we rely on absence of undefined values simply because there's a core checker that catches them preemptively for us.

[Quuxplusone]

Oh, I see it now. Actually I did some experiment with the test cstring-syntax.c in the trunk where it crashed. When I tried the same in release 8.0 it did not beacause unix.cstring.BadSizeArg did not depend on any other C string checker. However, now it depends on CStringModeling so it crashes. If I put the test to its proper place, bstring.c the crash goes away. The main difference between the two test files is indeed that in the invocations lines cstring-syntax.c omits the core checkers, which is wrong. It does not currently crash but it is non standard so I propose to include them there as well to prevent future misunderstandings and also to ensure correct testing of the unix.cstring checkers. I will do the fix if you agree.

[Quuxplusone]

I was thinking a bit on it. It is one thing to add core checkers in the test, but there should be no way for the user to crash the analyzer. Error messages yes, crash no. Somehow we must ensure that the correct dependencies are met.

One way is to add the dependencies one by one into the Checkers.td but the we should find out exactly which core checker is needed here to prevent the crash. The other way is to make the dependencies of the core checkers implicit for all other checkers (or at least all path sensitive checkers).

Maybe we should continue this discussion on the cfe-dev list?

[Quuxplusone]

https://reviews.llvm.org/D66042