[analyzer] In getSVal() API, disable auto-detection of void type as char type.

[NoQ]

Summary

In D38358, we ended up believing that reading the first byte of the void pointer is not the intended behavior of ProgramState::getSVal(Loc) when the type of the loaded value is not specified/hinted by the user and gets auto-detected as void. Therefore, instead of replacing the auto-detected void type with char and returning the first byte, fail with an easy-to-understand assertion to warn the user that it's better to specify the type explicitly in his case (even if he'd have to specify ACtx.CharTy anyway).

Additionally, allow specifying the type in the ProgramState::getSVal(const MemRegion *) override, because with the new assertion, a new use-case immediately shows up that requires such change. And, generally, it's a good idea to be able to specify the type here.

[danielmarjamaki]

LGTM

[xazax.hun]

I like the idea of making ProgramState::getSVal(const MemRegion *) symmetric to ProgramState::getSVal(Loc).

Just some philosophical questions which should probably be addressed in the future:
But also I wonder, should we maintain all these overloads? I mean, a lot of the APIs accept both SVal/Loc and MemRegion *. Usually, one of the implementations will end up just calling the other after some wrapping/unwrapping. Maybe ripping MemRegion * from the APIs would make it easier to use? E.g.: CallAndMessageChecker just unwraps the region from an SVal just to wrap it back again within the API call. However, we tend to use MemRegion * when storing info in the program state.

Otherwise LGTM!

include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h

307 ↗

(On Diff #118616)

The whitespace here is a bit off. I know it is not related to this patch, but this could be fixed if we already touch this file.

[NoQ]

Yeah, cleaning up this API would be great - as long as everybody loves to have the API broken and rewrite stuff.

If we are to simplify some APIs, i'd definitely start with getAsSymExpr()/getAsSymbol()/getAsSymbolicExpression().

Essentially we only need getSVal(MR) because only MR can be bound to, while other types of Loc cannot be bound to. Technically, only (base MR, offset) pairs can be bound to, but this is "implementation detail" that we're still trying to keep that way (even though it leaks like hell everywhere). So i'd rather think of the Loc overload as of a convenient wrapper (i.e., the user receives Loc in some checkLocation callback, so he wants to put it directly into the API to find his binding). And people would still need to operate with regions from time to time (eg., construct a sub-region manually when we expect it to be there, or strip casts).

Also the overload between getSVal(Ex, LC) and getSVal(R, Ty), when they do completely different things, seems confusing.

[dcoughlin]

LGTM. Thanks!

In D38801#910524, @NoQ wrote:

Also the overload between getSVal(Ex, LC) and getSVal(R, Ty), when they do completely different things, seems confusing.

Yes! I think we should change this (possibly rename them both?). I think it is a big barrier to entry -- I certainly found it super confusing when first getting started on the analyzer code base.

[NoQ]

Rebase on top of D39862 (attn. George!~).

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h

public:
   /// Returns UnknownVal() if none found.
   SVal getSVal(Loc LV, QualType T = QualType()) const;
 
   /// Returns the "raw" SVal bound to LV before any value simplfication.
   SVal getRawSVal(Loc LV, QualType T= QualType()) const;
 
   /// \brief Return the value bound to the specified location.
   /// Returns UnknownVal() if none found.
-  SVal getSVal(const MemRegion* R) const;
+  SVal getSVal(const MemRegion* R, QualType T = QualType()) const;
 
+  /// \brief Return the value bound to the specified location, assuming
+  /// that the value is a scalar integer or an enumeration or a pointer.
+  /// Returns UnknownVal() if none found or the region is not known to hold
+  /// a value of such type.
   SVal getSValAsScalarOrLoc(const MemRegion *R) const;
 
   /// \brief Visits the symbols reachable from the given SVal using the provided
   /// SymbolVisitor.
   ///
   /// This is a convenience API. Consider using ScanReachableSymbols class
   /// directly when making multiple scans on the same state with the same
   /// visitor to avoid repeated initialization cost.
ProgramState::getSValAsScalarOrLoc(const Stmt *S,
 
   return UnknownVal();
 }
 
 inline SVal ProgramState::getRawSVal(Loc LV, QualType T) const {
   return getStateManager().StoreMgr->getBinding(getStore(), LV, T);
 }
 
-inline SVal ProgramState::getSVal(const MemRegion* R) const {
+inline SVal ProgramState::getSVal(const MemRegion* R, QualType T) const {
   return getStateManager().StoreMgr->getBinding(getStore(),
-                                                loc::MemRegionVal(R));
+                                                loc::MemRegionVal(R),
+                                                T);
 }
 
 inline BasicValueFactory &ProgramState::getBasicVals() const {
   return getStateManager().getBasicVals();
 }
 
 inline SymbolManager &ProgramState::getSymbolManager() const {
   return getStateManager().getSymbolManager();

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

bool CallAndMessageChecker::uninitRefOrPointer(
   } else
     return false;
 
   if(!ParamDecl->getType()->getPointeeType().isConstQualified())
     return false;
 
   if (const MemRegion *SValMemRegion = V.getAsRegion()) {
     const ProgramStateRef State = C.getState();
-    const SVal PSV = State->getSVal(SValMemRegion);
+    const SVal PSV = State->getSVal(SValMemRegion, C.getASTContext().CharTy);
     if (PSV.isUndef()) {
       if (ExplodedNode *N = C.generateErrorNode()) {
         LazyInit_BT(BD, BT);
         auto R = llvm::make_unique<BugReport>(*BT, Os.str(), N);
         R->addRange(ArgRange);
         if (ArgEx) {
           bugreporter::trackNullOrUndefValue(N, ArgEx, *R);
         }

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

bool GenericTaintChecker::checkPre(const CallExpr *CE, CheckerContext &C) const{
 
   if (checkTaintedBufferSize(CE, FDecl, C))
     return true;
 
   return false;
 }
 
 Optional<SVal> GenericTaintChecker::getPointedToSVal(CheckerContext &C,
-                                            const Expr* Arg) {
+                                                     const Expr *Arg) {
   ProgramStateRef State = C.getState();
   SVal AddrVal = State->getSVal(Arg->IgnoreParens(), C.getLocationContext());
   if (AddrVal.isUnknownOrUndef())
     return None;
 
   Optional<Loc> AddrLoc = AddrVal.getAs<Loc>();
   if (!AddrLoc)
     return None;
 
-  const PointerType *ArgTy =
-    dyn_cast<PointerType>(Arg->getType().getCanonicalType().getTypePtr());
-  return State->getSVal(*AddrLoc, ArgTy ? ArgTy->getPointeeType(): QualType());
+  QualType ArgTy = Arg->getType().getCanonicalType();
+  if (!ArgTy->isPointerType())
+    return None;
+
+  QualType ValTy = ArgTy->getPointeeType();
+
+  // Do not dereference void pointers. Treat them as byte pointers instead.
+  // FIXME: we might want to consider more than just the first byte.
+  if (ValTy->isVoidType())
+    ValTy = C.getASTContext().CharTy;
+
+  return State->getSVal(*AddrLoc, ValTy);
 }
 
 ProgramStateRef
 GenericTaintChecker::TaintPropagationRule::process(const CallExpr *CE,
                                                    CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
   // Check for taint in arguments.

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

if (T.isNull()) {
       if (const TypedRegion *TR = dyn_cast<TypedRegion>(MR))
         T = TR->getLocationType()->getPointeeType();
       else if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(MR))
         T = SR->getSymbol()->getType()->getPointeeType();
       else if (isa<AllocaRegion>(MR))
         T = Ctx.VoidTy;
     }
     assert(!T.isNull() && "Unable to auto-detect binding type!");
-    if (T->isVoidType()) {
-      // When trying to dereference a void pointer, read the first byte.
-      T = Ctx.CharTy;
-    }
+    assert(!T->isVoidType() && "Attempting to dereference a void pointer!");
     MR = GetElementZeroRegion(cast<SubRegion>(MR), T);
   }
 
   // FIXME: Perhaps this method should just take a 'const MemRegion*' argument
   //  instead of 'Loc', and have the other Loc cases handled at a higher level.
   const TypedValueRegion *R = cast<TypedValueRegion>(MR);
   QualType RTy = R->getValueType();