[PATCH v4] Bug 14526 - MallocChecker: Add awareness of kernel malloc() flags
Conrad Meyer
cse.cem at gmail.com
Wed Mar 26 10:21:15 PDT 2014
Hm, good point. The unit tests did pass as-is. Given how you've
described CallocMem, though, I'm not sure why.
Thanks,
Conrad
On Wed, Mar 26, 2014 at 1:12 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> Um, Conrad, did you actually try this patch? CallocMem is set up to take sizes from the first and second arguments. I think you're going to want to call MallocMemAux directly.
>
> Committed with that change in r204832. Thanks for working on this!
>
> On Mar 21, 2014, at 11:05 , 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).
>>
>> This should reduce the number of false positives when running static
>> analysis on BSD kernels.
>>
>> 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.
>>
>> 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>
>> ---
>> Based on newish LLVM, Clang. Something more recently broke my build so I just
>> rebased to sources from Tuesday. Shouldn't be too stale.
>>
>> LLVM+Clang builds, and the new tests pass.
>>
>> Per review, renamed M_ZERO and MallocZero to KernelZeroFlagVal and
>> performKernelMalloc(). Also, dropped RetVal and used None for the aborted
>> returns in performKernelMalloc().
>>
>> I think I missed the mark earlier on the question:
>>
>>> Are there ever going to be systems that have both kmalloc and
>>> malloc-with-flags?
>>
>> I don't think there will be a system where both types of malloc are equally
>> important. For now I think it's safe to assume that one of the flag values is
>> more important than the other, if both exist simultaneously (__GFP_ZERO and
>> M_ZERO).
>>
>> Thanks.
>> ---
>> lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 114 ++++++++++++++++++++++++-
>> lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp | 1 +
>> test/Analysis/kmalloc-linux.c | 58 +++++++++++++
>> test/Analysis/malloc-three-arg.c | 58 +++++++++++++
>> 4 files changed, 227 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 f64c392..7ec777d 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> KernelZeroFlagVal;
>>
>> void initIdentifierInfo(ASTContext &C) const;
>>
>> /// \brief Determine family of a deallocation expression.
>> AllocationFamily getAllocationFamily(CheckerContext &C, const Stmt *S) const;
>> @@ -251,10 +255,16 @@ private:
>> static ProgramStateRef MallocMemAux(CheckerContext &C, const CallExpr *CE,
>> SVal SizeEx, SVal Init,
>> ProgramStateRef State,
>> AllocationFamily Family = AF_Malloc);
>>
>> + // Check if this malloc() for special flags. At present that means M_ZERO or
>> + // __GFP_ZERO (in which case, treat it like calloc).
>> + llvm::Optional<ProgramStateRef>
>> + performKernelMalloc(const CallExpr *CE, CheckerContext &C,
>> + const ProgramStateRef &State) const;
>> +
>> /// Update the RefState to reflect the new memory allocation.
>> static ProgramStateRef
>> MallocUpdateRefState(CheckerContext &C, const Expr *E, ProgramStateRef State,
>> AllocationFamily Family = AF_Malloc);
>>
>> @@ -480,10 +490,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;
>> @@ -506,11 +517,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>())
>> @@ -570,10 +581,85 @@ bool MallocChecker::isStandardNewDelete(const FunctionDecl *FD,
>>
>> // One of the standard new/new[]/delete/delete[] non-placement operators.
>> return true;
>> }
>>
>> +llvm::Optional<ProgramStateRef> MallocChecker::performKernelMalloc(
>> + 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();
>> +
>> + if (!KernelZeroFlagVal.hasValue()) {
>> + if (os == llvm::Triple::FreeBSD)
>> + KernelZeroFlagVal = 0x0100;
>> + else if (os == llvm::Triple::NetBSD)
>> + KernelZeroFlagVal = 0x0002;
>> + else if (os == llvm::Triple::OpenBSD)
>> + KernelZeroFlagVal = 0x0008;
>> + else if (os == llvm::Triple::Linux)
>> + // __GFP_ZERO
>> + KernelZeroFlagVal = 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 None;
>> + }
>> +
>> + // 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 None;
>> +
>> + 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 None;
>> + }
>> +
>> + NonLoc Flags = V.castAs<NonLoc>();
>> + NonLoc ZeroFlag = C.getSValBuilder()
>> + .makeIntVal(KernelZeroFlagVal.getValue(), FlagsEx->getType())
>> + .castAs<NonLoc>();
>> + SVal MaskedFlagsUC = C.getSValBuilder().evalBinOpNN(State, BO_And,
>> + Flags, ZeroFlag,
>> + FlagsEx->getType());
>> + if (MaskedFlagsUC.isUnknownOrUndef())
>> + return None;
>> + 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)
>> + return CallocMem(C, CE);
>> +
>> + return None;
>> +}
>> +
>> void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const {
>> if (C.wasInlined)
>> return;
>>
>> const FunctionDecl *FD = C.getCalleeDecl(CE);
>> @@ -585,11 +671,31 @@ 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 =
>> + performKernelMalloc(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 =
>> + performKernelMalloc(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