[PATCH v2] Bug 14526 - Make malloc() static analysis M_ZERO-aware

[Jordan Rose]

Seeing this code is really making me think we need to expose a way to look up macros in the analyzer, even if it only handles simple ones...

On Mar 14, 2014, at 9:53 , Meyer, Conrad <conrad.meyer at isilon.com> wrote:

> Add M_ZERO awareness to malloc() static analysis in Clang for FreeBSD,
> NetBSD, and OpenBSD 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.
> 
> Add Analysis/malloc-three-arg.c tests for M_ZERO-alike malloc.
> 
> Sponsored by: EMC/Isilon storage division
> Signed-off-by: Conrad Meyer <conrad.meyer at isilon.com>
> ---
> lib/StaticAnalyzer/Checkers/MallocChecker.cpp  | 69 +++++++++++++++++++++++++-
> lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp |  1 +
> test/Analysis/malloc-three-arg.c               | 58 ++++++++++++++++++++++
> 3 files changed, 127 insertions(+), 1 deletion(-)
> create mode 100644 test/Analysis/malloc-three-arg.c
> 
> diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
> index ca40beb..274ae5a 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,72 @@ 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) {
> +        // 3-argument malloc(), as commonly used in {Free,Net,Open}BSD Kernels:
> +        //
> +        // void *malloc(unsigned long size, struct malloc_type *mtp, int flags);
> +        //
> +        // One of the possible flags is M_ZERO, which means 'give me back an
> +        // allocation which is already zeroed', like calloc.
> +
> +        // This logic is largely cloned from O_CREAT in UnixAPIChecker, maybe some
> +        // code could be shared.
> +
> +        if (!Val_M_ZERO.hasValue()) {
> +          if (C.getASTContext().getTargetInfo().getTriple().getOS()
> +                                                            == llvm::Triple::FreeBSD)
> +            Val_M_ZERO = 0x0100;
> +          else if (C.getASTContext().getTargetInfo().getTriple().getOS()
> +                                                                 == llvm::Triple::NetBSD)
> +            Val_M_ZERO = 0x0002;
> +          else if (C.getASTContext().getTargetInfo().getTriple().getOS()
> +                                                                 == llvm::Triple::OpenBSD)
> +            Val_M_ZERO = 0x0008;

Let's factor that common call out of the if-statement, so we don't have to wrap it. (The LLVM way to wrap it would be to put the == at the end of the line, then break, then indent to just past the open paren.)

> +          else
> +            // FIXME: We need a more general way of getting the M_ZERO value.
> +            // See also: O_CREAT in UnixAPIChecker.cpp.
> +
> +            // Fall back to normal malloc behavior on platforms where we don't
> +            // know M_ZERO.
> +            State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
> +        }
> +
> +        if (Val_M_ZERO.hasValue()) {
> +          const Expr *FlagsEx = CE->getArg(2);
> +          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;

This could be unknown if the argument value is unknown (i.e. the analyzer has failed to symbolize it for some reason). In that case, we should probably still treat this as a regular malloc.

> +          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);
> +        }
> +      }

The "fall-back" case has been duplicated twice already, and I just suggested adding it a third time. How about adding a helper function for three-argument malloc that returns an Optional<ProgramStateRef>, and if it returns None, we just do the normal malloc thing? That way we can use early returns to simplify all this.

(Why Optional<ProgramStateRef>? In case there's ever a reason for three-argument malloc to return a null state, i.e. it detects some state that shouldn't be reached.)


> +    } else if (FunI == II_valloc) {
>       if (CE->getNumArgs() < 1)
>         return;
>       State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);

This is generally looking good. Thanks for bearing with me on these change requests!

Jordan