[analyzer] do not crash when trying to convert an APSInt to an unexpected type

[george.karpenkov]

Summary

This is the issue breaking the postgresql bot, purely by chance exposed through taint checker, somehow appearing after https://reviews.llvm.org/D38358 got committed.

The backstory is that the taint checker requests SVal for the value of the pointer, and analyzer has a "fast path" in the getter to return a constant when we know that the value is constant.
Unfortunately, the getter requires a cast to get signedness correctly, and for the pointer void * the cast crashes.

This is more of a band-aid patch, as I am not sure what could be done here "correctly", but it should be applied in any case to avoid the crash.

@dcoughlin faster review here would be appreciated to get the bot back to green.

[dcoughlin]

This seems reasonable to me. Please commit it. @NoQ can do a post-commit review and fix it up if he would rather address the issue differently.

[NoQ]

I'm curious if the crash would turn into an assertion failure during getRawSVal() after D38801 is committed.

The problem with this checker producing void symbols is known since D26837. If this problem is fixed on the checker side (it probably should be - the checker , we can probably put stronger asserts onto types suitable for symbols.

lib/StaticAnalyzer/Core/ProgramState.cpp

265 ↗

(On Diff #122306)

If a type is an integral or enumeration type or a Loc type, then it is definitely not null.

[george.karpenkov]

I'm curious if the crash would turn into an assertion failure during getRawSVal() after D38801 is committed.

I guess we would see?

If this problem is fixed on the checker side (it probably should be - the checker , we can probably put stronger asserts onto types suitable for symbols.

Of course I'm new, but I disagree with this statement: in order to have a robust API, the function should not crash, unless the caller violates an explicit precondition.
getSVal is just a function for getting a symbolic value for a particular statement, it seems totally valid to query it for an expression which type is void.

lib/StaticAnalyzer/Core/ProgramState.cpp

265 ↗

(On Diff #122306)

But the check itself will crash if the type is null.

[NoQ]

In D39862#921075, @george.karpenkov wrote:

Of course I'm new, but I disagree with this statement: in order to have a robust API, the function should not crash, unless the caller violates an explicit precondition.
getSVal is just a function for getting a symbolic value for a particular statement, it seems totally valid to query it for an expression which type is void.

Not for an expression - this overload retrieves a value stored in the memory region. I think that we should inform the checker's author when he tries to interpret the value in memory as a void value, because i believe that in pretty much all cases he has a better type to provide.

lib/StaticAnalyzer/Core/ProgramState.cpp

265 ↗

(On Diff #122306)

Whoops right sry.

cfe/trunk/lib/StaticAnalyzer/Core/ProgramState.cpp

 
 SVal ProgramState::getSVal(Loc location, QualType T) const {
   SVal V = getRawSVal(cast<Loc>(location), T);
 
   // If 'V' is a symbolic value that is *perfectly* constrained to
   // be a constant value, use that value instead to lessen the burden
   // on later analysis stages (so we have less symbolic values to reason
   // about).
-  if (!T.isNull()) {
+  // We only go into this branch if we can convert the APSInt value we have
+  // to the type of T, which is not always the case (e.g. for void).
+  if (!T.isNull() && (T->isIntegralOrEnumerationType() || Loc::isLocType(T))) {
     if (SymbolRef sym = V.getAsSymbol()) {
       if (const llvm::APSInt *Int = getStateManager()
                                     .getConstraintManager()
                                     .getSymVal(this, sym)) {
         // FIXME: Because we don't correctly model (yet) sign-extension
         // and truncation of symbolic values, we need to convert
         // the integer value to the correct signedness and bitwidth.
         //

cfe/trunk/test/Analysis/taint-tester.c

void atoiTest() {
   long l = atol(s); // expected-warning + {{tainted}}
   int tl = l; // expected-warning + {{tainted}}
 
   long long ll = atoll(s); // expected-warning + {{tainted}}
   int tll = ll; // expected-warning + {{tainted}}
 
 }
 
+char *pointer1;
+void *pointer2;
+void noCrashTest() {
+  if (!*pointer1) {
+    __builtin___memcpy_chk(pointer2, pointer1, 0, 0); // no-crash
+  }
+}