[PATCH] Fix bug 14526 - Make malloc() static analysis M_ZERO-aware

Jordan Rose jordan_rose at apple.com
Thu Mar 13 20:46:35 PDT 2014


[-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 <conrad.meyer at isilon.com> 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 <conrad.meyer at isilon.com>
> ---
> 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




More information about the cfe-commits mailing list