[analyzer] Do not attempt to get the pointee of void* .

[alexander-shaposhnikov]

Summary

Do not attempt to get the pointee of void* while generating a bug report
(otherwise it will trigger an assert inside RegionStoreManager::getBinding
assert(!T->isVoidType() && "Attempting to dereference a void pointer!")).
(the call stack:

(anonymous namespace)::RegionStoreManager::getBinding
clang::ento::ProgramState::getSVal
clang::ento::StackHintGeneratorForSymbol::getMessage
clang::ento::PathDiagnosticEventPiece::getCallStackMessage 
....

)
Test plan: make check-all

[alexander-shaposhnikov]

indents

[NoQ]

Hmm, the original code seems quite quick-and-dirty. It, indeed, tries to blindly dereference something that's accidentally "some pointer".

Could you change the type check to consider AST types instead, i.e. check that (*I)->getType() is a double-pointer type? And then pass its single-pointee type into getSVal as the second optional argument, so that it knew what to expect. This would be the correct way to express what the author was trying to say.

Because your code ignores possible layers of sub-regions contained within Reg, and i'm afraid we may loose notes because of that.

(in any case there are many more ways in which a pointer can be passed into a function that this visitor doesn't pattern-match, but that's not super problematic)

[alexander-shaposhnikov]

switched to checking (*I)->getType()

[NoQ]

Yeah, i guess that'd work as well :)

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

for (CallExpr::const_arg_iterator I = CE->arg_begin(),
     // Check if the variable corresponding to the symbol is passed by value.
     SymbolRef AS = SV.getAsLocSymbol();
     if (AS == Sym) {
       return getMessageForArg(*I, ArgIndex);
     }
 
     // Check if the parameter is a pointer to the symbol.
     if (Optional<loc::MemRegionVal> Reg = SV.getAs<loc::MemRegionVal>()) {
+      // Do not attempt to dereference void*.
+      if (const auto *S = Reg->getAsSymbol())
+        if (S->getType().getCanonicalType()->isVoidPointerType())
+          continue;
       SVal PSV = N->getState()->getSVal(Reg->getRegion());
       SymbolRef AS = PSV.getAsLocSymbol();
       if (AS == Sym) {
         return getMessageForArg(*I, ArgIndex);
       }
     }
   }
 

test/Analysis/malloc.c

 // of the C-string checker.
 void cstringchecker_bounds_nocrash() {
   char *p = malloc(2);
   strncpy(p, "AAA", sizeof("AAA")); // expected-warning {{Size argument is greater than the length of the destination buffer}}
 
   free(p);
 }
 
+void allocateSomeMemory(void *offendingParameter, void** ptr) {
+  *ptr = malloc(1);
+}
+
+void testNoCrashOnOffendingParameter() {
+  // "extern" is necessary to avoid unrelated warnings 
+  // on passing uninitialized value.
+  extern void *offendingParameter;
+  void* ptr;
+  allocateSomeMemory(offendingParameter, &ptr);
+} // expected-warning {{Potential leak of memory pointed to by 'ptr'}}
+
 // ----------------------------------------------------------------------------
 // False negatives.
 
 void testMallocWithParam(int **p) {
   *p = (int*) malloc(sizeof(int));
   *p = 0; // FIXME: should warn here
 }