[-llvm-commits] Hi, Conrad. Thanks for picking this up. I have a few comments on the patch, and of course you'll need to include a regression test too before it can be committed. You can see how these work in test/Analysis/malloc.c; feel free to make a new test file so that you can declare the kernel version of malloc without conflicting with the normal one.
On Mar 13, 2014, at 10:28 , Meyer, Conrad <[email protected]> wrote: > Add M_ZERO awareness to malloc() static analysis in Clang for FreeBSD, > in a similar fashion to O_CREAT for open(2). There is some opportunity > for code-sharing here. > > This should reduce the number of false positives when running static > analysis on FreeBSD. > > Future work involves a better (symbolic) method of checking for named > flags without hardcoding values. > > Sponsored by: EMC/Isilon storage division > Signed-off-by: Conrad Meyer <[email protected]> > --- > lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 52 +++++++++++++++++++++++++- > lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp | 1 + > 2 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > index ca40beb..b89a6a3 100644 > --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > @@ -16,6 +16,7 @@ > #include "InterCheckerAPI.h" > #include "clang/AST/Attr.h" > #include "clang/Basic/SourceManager.h" > +#include "clang/Basic/TargetInfo.h" > #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" > #include "clang/StaticAnalyzer/Core/Checker.h" > #include "clang/StaticAnalyzer/Core/CheckerManager.h" > @@ -208,6 +209,7 @@ private: > mutable std::unique_ptr<BugType> BT_OffsetFree[CK_NumCheckKinds]; > mutable IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc, > *II_valloc, *II_reallocf, *II_strndup, *II_strdup; > + mutable Optional<uint64_t> Val_M_ZERO; > > void initIdentifierInfo(ASTContext &C) const; > > @@ -586,7 +588,55 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, > CheckerContext &C) const { > initIdentifierInfo(C.getASTContext()); > IdentifierInfo *FunI = FD->getIdentifier(); > > - if (FunI == II_malloc || FunI == II_valloc) { > + if (FunI == II_malloc) { > + if (CE->getNumArgs() < 1) > + return; > + if (CE->getNumArgs() < 2) { > + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State); > + } else if (CE->getNumArgs() == 3) { > + // This logic is largely cloned from O_CREAT in UnixAPIChecker, > maybe some > + // code could be shared. Most people have probably never seen a three-argument malloc. Mentioning that it's from the FreeBSD kernel (and possibly other kernels?) and possibly including the declaration in a comment would go a long way towards explaining why this code is here at all. > + > + if (!Val_M_ZERO.hasValue()) { > + if (C.getASTContext().getTargetInfo().getTriple().getOS() > + == > llvm::Triple::FreeBSD) > + Val_M_ZERO = 0x00100; > + else > + // FIXME: We need a more general way of getting the M_ZERO value. > + // See also: O_CREAT in UnixAPIChecker.cpp. > + return; > + } I don't think "return" is appropriate here. Isn't it better to fall back to regular malloc() if we don't know what M_ZERO is on this platform? (In the O_CREAT case, the warning was only about misusing O_CREAT, so it didn't matter that we were bailing out early.) > + > + ProgramStateRef state = C.getState(); > + const Expr *flagsEx = CE->getArg(2); Please follow the LLVM naming conventions: capital letters for all variables as well as types. (This applies further down as well.) > + const SVal V = state->getSVal(flagsEx, C.getLocationContext()); > + if (!V.getAs<NonLoc>()) { > + // The case where 'V' can be a location can only be due to a bad > header, > + // so in this case bail out. > + return; > + } > + > + NonLoc flags = V.castAs<NonLoc>(); > + NonLoc zeroFlag = C.getSValBuilder() > + .makeIntVal(Val_M_ZERO.getValue(), > flagsEx->getType()).castAs<NonLoc>(); > + SVal maskedFlagsUC = C.getSValBuilder().evalBinOpNN(state, BO_And, > + flags, zeroFlag, > + > flagsEx->getType()); > + if (maskedFlagsUC.isUnknownOrUndef()) > + return; > + DefinedSVal maskedFlags = maskedFlagsUC.castAs<DefinedSVal>(); > + > + // Check if maskedFlags is non-zero. > + ProgramStateRef trueState, falseState; > + std::tie(trueState, falseState) = state->assume(maskedFlags); > + > + // If M_ZERO is set, treat this like calloc (initialized). > + if (!(trueState && !falseState)) > + State = CallocMem(C, CE); > + else > + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State); You need to be a bit more careful here...the if-check is exactly backwards from what you want. In more detail, there are three possible cases: - M_ZERO is known to be set: trueState exists, but falseState doesn't. - M_ZERO is known not to be set: falseState exists, but trueState doesn't - The analyzer isn't sure: both states exist In the latter two states, we should probably assume the existing behavior (malloc returns undefined). > + } > + } else if (FunI == II_valloc) { > if (CE->getNumArgs() < 1) > return; > State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State); > diff --git a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp > b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp > index 8869654..aa7e25bc 100644 > --- a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp > +++ b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp > @@ -80,6 +80,7 @@ void UnixAPIChecker::CheckOpen(CheckerContext &C, const > CallExpr *CE) const { > // FIXME: We need a more general way of getting the O_CREAT value. > // We could possibly grovel through the preprocessor state, but > // that would require passing the Preprocessor object to the ExprEngine. > + // See also: MallocChecker.cpp / M_ZERO. > return; > } > } Thanks for working on this! Jordan _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
