[analyzer] Fix SimpleSValBuilder evalBinOpLN elementType might be nullptr issue

[xiangzhai]

Summary

Hi LLVM developers,

When I use analyzer to static analysis k3b for detecting Memory-leak, Use-after-free and sort of issues, it segfault! so I debug the analyzer to fix the issue.

Testcase:

clang++ -cc1 -triple x86_64-isoft-linux -analyze -disable-free -disable-llvm-verifier -discard-value-names -main-file-name k3bdevice.cpp -analyzer-store=region -analyzer-opt-analyze-nested-blocks -analyzer-eagerly-assume -analyzer-checker=core -analyzer-checker=apiModeling -analyzer-checker=unix -analyzer-checker=deadcode -analyzer-checker=cplusplus -analyzer-checker=security.insecureAPI.UncheckedReturn -analyzer-checker=security.insecureAPI.getpw -analyzer-checker=security.insecureAPI.gets -analyzer-checker=security.insecureAPI.mktemp -analyzer-checker=security.insecureAPI.mkstemp -analyzer-checker=security.insecureAPI.vfork -analyzer-checker=nullability.NullPassedToNonnull -analyzer-checker=nullability.NullReturnedFromNonnull -analyzer-output plist -w -mrelocation-model pic -pic-level 2 -mthread-model posix -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 -dwarf-column-info -debugger-tuning=gdb -resource-dir /usr/lib/clang/5.0.0 -isystem /usr/local/include -D HAVE_QT5WEBKITWIDGETS -D KCOREADDONS_LIB -D QT_CORE_LIB -D QT_NO_DEBUG -D QT_NO_URL_CAST_FROM_STRING -D _GNU_SOURCE -D _LARGEFILE64_SOURCE -D k3bdevice_EXPORTS -internal-isystem /usr/local/include -internal-isystem /usr/lib/clang/5.0.0/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -Wno-long-long -std=gnu++11 -fdeprecated-macro -ferror-limit 19 -fmessage-length 0 -fvisibility hidden -fvisibility-inlines-hidden -fno-operator-names -fobjc-runtime=gcc -fdiagnostics-show-option -x c++ k3bdevice.cpp

Because the preprocessed fileSize is 2.3MB, please download it. and review my patch, thanks a lot!

Regards,
Leslie Zhai

[NoQ]

Hello,

Thanks for looking into this!
I've heard that this may fail, but i didn't have a good reproduction case. I might also have fixed it partially or completely as a side effect of https://reviews.llvm.org/rL298927 .

The most interesting thing here is to explain why elementType becomes null - whether it should actually be null from time to time, or if its being null is an error in itself that should be fixed. Your patch essentially removes the null check, however it is better to address the root cause of the issue. It would also allow to reduce the test case to a few lines of code and to add it into our test suite, which is necessary anyway.

These sorts of problems are usually debugged by seeking answers to the following questions:

• What is the expression that is being evaluated symbolically when we crash? (it can be easily ->dump()ed from a debugger).
• What are SVals for the sub-expressions of this expressions, and what binary operation are we evaluating over them?
• By looking at the above, what would be the correct SVal to represent the result of this operation? (the answer may be non-trivial if any)
• *do things to produce such SVal*

[xiangzhai]

Thanks for your review! I am on vacation until Wednesday this week, then I will update my patch :)

[NoQ]

Hmm. I could not instantly reproduce the crash on your k3bdevice.cpp example, it doesn't crash both with and without both your and my patches.

[xiangzhai]

Hi Artem,

Thanks for your reply!

Hmm. I could not instantly reproduce the crash on your k3bdevice.cpp example, it doesn't crash both with and without both your and my patches.

Sorry, I forgot to say that you need to patch -Np1 -i Glib-MallocChecker-two-arguments.patch to reproduce the crash!

The bt show about that, so I thought you have already patched it:

#15 0x0000000002fb838b (anonymous namespace)::MallocChecker::SValBinMulOp(clang::ento::CheckerContext&, clang::Expr const*, clang::Expr const*, llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>) /data/project/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:795:32

The most interesting thing here is to explain why elementType becomes null

It is my fault to bring in the issue, please carefully review my patch to help me addressing the root cause of the issue:

SVal TotalSize = svalBuilder.evalBinOp(State, BO_Mul, nBlocks, nBlockBytes, 
                                                                  svalBuilder.getContext().getSizeType());

Thanks,
Leslie Zhai

[NoQ]

Oh, sorry, i didn't notice this detail. I see it now.

evalBinOpLN() performs operations on Loc and NonLoc, the only kind of operation that you'd expect it to perform is adding an integer to a pointer (or subtracting an integer from a pointer). That's not what you wanted in MallocChecker, so you shouldn't have called this at all. I'd comment on the other review then.

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

else if (isa<SubRegion>(region)) {
       assert(op == BO_Add || op == BO_Sub);
       index = (op == BO_Add) ? rhs : evalMinus(rhs);
       superR = region;
       if (resultTy->isAnyPointerType())
         elementType = resultTy->getPointeeType();
     }
 
     if (Optional<NonLoc> indexV = index.getAs<NonLoc>()) {
+      if (!elementType.isNull())
         return loc::MemRegionVal(MemMgr.getElementRegion(elementType, *indexV,
                                                          superR, getContext()));
     }
   }
   return UnknownVal();
 }
 
 const llvm::APSInt *SimpleSValBuilder::getKnownValue(ProgramStateRef state,
                                                    SVal V) {
   if (V.isUnknownOrUndef())