[PATCH v3] Bug 14526 - Make malloc() static analysis M_ZERO-aware
Jordan Rose
jordan_rose at apple.com
Fri Mar 21 10:11:25 PDT 2014
On Mar 20, 2014, at 6:08 , 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.
>
> Additionally, add kmalloc() (Linux kernel malloc()) and treat __GFP_ZERO
> like M_ZERO on Linux.
>
> Future work involves a better (symbolic) method of checking for named
> flags without hardcoding values, and treating kfree() (Linux kernel free())
> like free().
>
> Add Analysis/malloc-three-arg.c tests for M_ZERO-alike malloc, and
> Analysis/kmalloc-linux.c tests for the kmalloc() and __GFP_ZERO
> additions.
>
> Sponsored by: EMC/Isilon storage division
> Signed-off-by: Conrad Meyer <conrad.meyer at isilon.com>
> ---
> I rebased against master. It builds and new tests pass locally (Linux x86_64).
>
> I moved the "malloc with zero flag" functionality into a method, MallocZero().
> I'm not sure it had to be a method, but it certainly seems like this class is
> contained entirely in this CU so I'm not sure it matters too much. (The
> alternative was adding more arguments to MallocZero().)
>
> I added kmalloc() to MallocChecker and treat the similar __GFP_ZERO flag like
> M_ZERO. If this should be broken out into a seperate patch, that's easy enough
> to do. Just let me know. I also added a small test case, kmalloc-linux.c.
>
> And, I am a C programmer. I've probably made some C++ amateur mistakes. Sorry.
>
> Thanks for the kind reviews,
> Conrad
> ---
> lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 111 ++++++++++++++++++++++++-
> lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp | 1 +
> test/Analysis/kmalloc-linux.c | 58 +++++++++++++
> test/Analysis/malloc-three-arg.c | 58 +++++++++++++
> 4 files changed, 224 insertions(+), 4 deletions(-)
> create mode 100644 test/Analysis/kmalloc-linux.c
> 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..ae8d95f 100644
> --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
> +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
> @@ -14,10 +14,11 @@
>
> #include "ClangSACheckers.h"
> #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"
> #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
> #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
> @@ -155,11 +156,12 @@ class MallocChecker : public Checker<check::DeadSymbols,
> check::Location,
> eval::Assume>
> {
> public:
> MallocChecker() : II_malloc(0), II_free(0), II_realloc(0), II_calloc(0),
> - II_valloc(0), II_reallocf(0), II_strndup(0), II_strdup(0) {}
> + II_valloc(0), II_reallocf(0), II_strndup(0), II_strdup(0),
> + II_kmalloc(0) {}
>
> /// In pessimistic mode, the checker assumes that it does not know which
> /// functions might free the memory.
> enum CheckKind {
> CK_MallocPessimistic,
> @@ -205,11 +207,13 @@ private:
> mutable std::unique_ptr<BugType> BT_UseFree[CK_NumCheckKinds];
> mutable std::unique_ptr<BugType> BT_BadFree[CK_NumCheckKinds];
> mutable std::unique_ptr<BugType> BT_MismatchedDealloc;
> 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;
> + *II_valloc, *II_reallocf, *II_strndup, *II_strdup,
> + *II_kmalloc;
> + mutable Optional<uint64_t> Val_M_ZERO;
Let's rename this something generic, like KernelZeroFlagVal;
>
> void initIdentifierInfo(ASTContext &C) const;
>
> /// \brief Determine family of a deallocation expression.
> AllocationFamily getAllocationFamily(CheckerContext &C, const Stmt *S) const;
> @@ -251,10 +255,15 @@ private:
> static ProgramStateRef MallocMemAux(CheckerContext &C, const CallExpr *CE,
> SVal SizeEx, SVal Init,
> ProgramStateRef State,
> AllocationFamily Family = AF_Malloc);
>
> + // Check if this malloc() uses M_ZERO or __GFP_ZERO (treat like calloc).
> + llvm::Optional<ProgramStateRef>
> + MallocZero(const CallExpr *CE, CheckerContext &C,
> + const ProgramStateRef &State) const;
> +
How about something like "performKernelMalloc"? The special thing here is that it handles the two- and three-argument forms if it knows how. We may want to add support for the other flags some day.
Are there ever going to be systems that have both kmalloc and malloc-with-flags?
> /// Update the RefState to reflect the new memory allocation.
> static ProgramStateRef
> MallocUpdateRefState(CheckerContext &C, const Expr *E, ProgramStateRef State,
> AllocationFamily Family = AF_Malloc);
>
> @@ -479,10 +488,11 @@ void MallocChecker::initIdentifierInfo(ASTContext &Ctx) const {
> II_reallocf = &Ctx.Idents.get("reallocf");
> II_calloc = &Ctx.Idents.get("calloc");
> II_valloc = &Ctx.Idents.get("valloc");
> II_strdup = &Ctx.Idents.get("strdup");
> II_strndup = &Ctx.Idents.get("strndup");
> + II_kmalloc = &Ctx.Idents.get("kmalloc");
> }
>
> bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const {
> if (isFreeFunction(FD, C))
> return true;
> @@ -505,11 +515,11 @@ bool MallocChecker::isAllocationFunction(const FunctionDecl *FD,
> IdentifierInfo *FunI = FD->getIdentifier();
> initIdentifierInfo(C);
>
> if (FunI == II_malloc || FunI == II_realloc ||
> FunI == II_reallocf || FunI == II_calloc || FunI == II_valloc ||
> - FunI == II_strdup || FunI == II_strndup)
> + FunI == II_strdup || FunI == II_strndup || FunI == II_kmalloc)
> return true;
> }
>
> if (ChecksEnabled[CK_MallocOptimistic] && FD->hasAttrs())
> for (const auto *I : FD->specific_attrs<OwnershipAttr>())
> @@ -569,10 +579,85 @@ bool MallocChecker::isStandardNewDelete(const FunctionDecl *FD,
>
> // One of the standard new/new[]/delete/delete[] non-placement operators.
> return true;
> }
>
> +llvm::Optional<ProgramStateRef> MallocChecker::MallocZero(const CallExpr *CE,
> + CheckerContext &C, const ProgramStateRef &State) const {
> + // 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.
> +
> + // 2-argument kmalloc(), as used in the Linux kernel:
> + //
> + // void *kmalloc(size_t size, gfp_t flags);
> + //
> + // Has the similar flag value __GFP_ZERO.
> +
> + // This logic is largely cloned from O_CREAT in UnixAPIChecker, maybe some
> + // code could be shared.
> +
> + llvm::Triple::OSType os = C.getASTContext().getTargetInfo().getTriple().getOS();
> + llvm::Optional<ProgramStateRef> RetVal;
> +
> + if (!Val_M_ZERO.hasValue()) {
> + if (os == llvm::Triple::FreeBSD)
> + Val_M_ZERO = 0x0100;
> + else if (os == llvm::Triple::NetBSD)
> + Val_M_ZERO = 0x0002;
> + else if (os == llvm::Triple::OpenBSD)
> + Val_M_ZERO = 0x0008;
> + else if (os == llvm::Triple::Linux)
> + // __GFP_ZERO
> + Val_M_ZERO = 0x8000;
> + 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.
> + return RetVal;
This can be written "return None;". "None" implicitly converts to an empty Optional.
If you use this everywhere, you don't need to predeclare RetVal.
Other than that this looks pretty good!
Jordan
> + }
> +
> + // We treat the last argument as the flags argument, and callers fall-back to
> + // normal malloc on a None return. This works for the FreeBSD kernel malloc
> + // as well as Linux kmalloc.
> + if (CE->getNumArgs() < 2)
> + return RetVal;
> +
> + const Expr *FlagsEx = CE->getArg(CE->getNumArgs() - 1);
> + 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 RetVal;
> + }
> +
> + 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 RetVal;
> + 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)
> + RetVal = CallocMem(C, CE);
> +
> + return RetVal;
> +}
> +
> void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const {
> if (C.wasInlined)
> return;
>
> const FunctionDecl *FD = C.getCalleeDecl(CE);
> @@ -584,11 +669,29 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const {
>
> if (FD->getKind() == Decl::Function) {
> 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() < 3) {
> + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
> + } else if (CE->getNumArgs() == 3) {
> + llvm::Optional<ProgramStateRef> MaybeState = MallocZero(CE, C, State);
> + if (MaybeState.hasValue())
> + State = MaybeState.getValue();
> + else
> + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
> + }
> + } else if (FunI == II_kmalloc) {
> + llvm::Optional<ProgramStateRef> MaybeState = MallocZero(CE, C, State);
> + if (MaybeState.hasValue())
> + State = MaybeState.getValue();
> + else
> + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
> + } else if (FunI == II_valloc) {
> if (CE->getNumArgs() < 1)
> return;
> State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
> } else if (FunI == II_realloc) {
> State = ReallocMem(C, CE, false);
> 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
> @@ -78,10 +78,11 @@ void UnixAPIChecker::CheckOpen(CheckerContext &C, const CallExpr *CE) const {
> Val_O_CREAT = 0x0200;
> else {
> // 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;
> }
> }
>
> // Look at the 'oflags' argument for the O_CREAT flag.
> diff --git a/test/Analysis/kmalloc-linux.c b/test/Analysis/kmalloc-linux.c
> new file mode 100644
> index 0000000..87c1107
> --- /dev/null
> +++ b/test/Analysis/kmalloc-linux.c
> @@ -0,0 +1,58 @@
> +// RUN: %clang -target x86_64-unknown-linux --analyze %s
> +
> +#include "Inputs/system-header-simulator.h"
> +
> +#define __GFP_ZERO 0x8000
> +#define NULL ((void *)0)
> +
> +void *kmalloc(size_t, int);
> +
> +struct test {
> +};
> +
> +void foo(struct test *);
> +
> +void test_zeroed() {
> + struct test **list, *t;
> + int i;
> +
> + list = kmalloc(sizeof(*list) * 10, __GFP_ZERO);
> + if (list == NULL)
> + return;
> +
> + for (i = 0; i < 10; i++) {
> + t = list[i];
> + foo(t);
> + }
> + free(list); // no-warning
> +}
> +
> +void test_nonzero() {
> + struct test **list, *t;
> + int i;
> +
> + list = kmalloc(sizeof(*list) * 10, 0);
> + if (list == NULL)
> + return;
> +
> + for (i = 0; i < 10; i++) {
> + t = list[i]; // expected-warning{{undefined}}
> + foo(t);
> + }
> + free(list);
> +}
> +
> +void test_indeterminate(int flags) {
> + struct test **list, *t;
> + int i;
> +
> + list = kmalloc(sizeof(*list) * 10, flags);
> + if (list == NULL)
> + return;
> +
> + for (i = 0; i < 10; i++) {
> + t = list[i]; // expected-warning{{undefined}}
> + foo(t);
> + }
> + free(list);
> +}
> diff --git a/test/Analysis/malloc-three-arg.c b/test/Analysis/malloc-three-arg.c
> new file mode 100644
> index 0000000..01b08ae
> --- /dev/null
> +++ b/test/Analysis/malloc-three-arg.c
> @@ -0,0 +1,58 @@
> +// RUN: %clang -target x86_64-unknown-freebsd --analyze %s
> +
> +#include "Inputs/system-header-simulator.h"
> +
> +#define M_ZERO 0x0100
> +#define NULL ((void *)0)
> +
> +void *malloc(size_t, void *, int);
> +
> +struct test {
> +};
> +
> +void foo(struct test *);
> +
> +void test_zeroed() {
> + struct test **list, *t;
> + int i;
> +
> + list = malloc(sizeof(*list) * 10, NULL, M_ZERO);
> + if (list == NULL)
> + return;
> +
> + for (i = 0; i < 10; i++) {
> + t = list[i];
> + foo(t);
> + }
> + free(list); // no-warning
> +}
> +
> +void test_nonzero() {
> + struct test **list, *t;
> + int i;
> +
> + list = malloc(sizeof(*list) * 10, NULL, 0);
> + if (list == NULL)
> + return;
> +
> + for (i = 0; i < 10; i++) {
> + t = list[i]; // expected-warning{{undefined}}
> + foo(t);
> + }
> + free(list);
> +}
> +
> +void test_indeterminate(int flags) {
> + struct test **list, *t;
> + int i;
> +
> + list = malloc(sizeof(*list) * 10, NULL, flags);
> + if (list == NULL)
> + return;
> +
> + for (i = 0; i < 10; i++) {
> + t = list[i]; // expected-warning{{undefined}}
> + foo(t);
> + }
> + free(list);
> +}
> --
> 1.8.5.3
More information about the cfe-commits
mailing list