Index: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -11,9 +11,13 @@ // //===----------------------------------------------------------------------===// -#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/AST/ComputeDependence.h" +#include "clang/AST/Decl.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/Stmt.h" +#include "clang/Basic/LLVM.h" #include "clang/Basic/SourceManager.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" @@ -27,8 +31,7 @@ namespace { class StackAddrEscapeChecker - : public Checker, - check::EndFunction> { + : public Checker, check::EndFunction> { mutable IdentifierInfo *dispatch_semaphore_tII; mutable std::unique_ptr BT_stackleak; mutable std::unique_ptr BT_returnstack; @@ -46,10 +49,15 @@ CheckerNameRef CheckNames[CK_NumCheckKinds]; void checkPreCall(const CallEvent &Call, CheckerContext &C) const; - void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const; + void checkPreReturnStmt(const ReturnStmt *RS, CheckerContext &C) const; + void checkPreStmt(const Stmt *S, CheckerContext &C) const; void checkEndFunction(const ReturnStmt *RS, CheckerContext &Ctx) const; private: + void checkAssignmentStmtForStackObjEscapeToPtrOutParam( + const BinaryOperator *Assignment, CheckerContext &C) const; + void checkAssignmentStmtForStackArrayEscapeToPtrOutParam( + const BinaryOperator *Assignment, CheckerContext &C) const; void checkReturnedBlockCaptures(const BlockDataRegion &B, CheckerContext &C) const; void checkAsyncExecutedBlockCaptures(const BlockDataRegion &B, @@ -249,8 +257,8 @@ } } -void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, - CheckerContext &C) const { +void StackAddrEscapeChecker::checkPreReturnStmt(const ReturnStmt *RS, + CheckerContext &C) const { if (!ChecksEnabled[CK_StackAddrEscapeChecker]) return; @@ -288,9 +296,365 @@ } } + // TODO how about returning a global variable? EmitStackError(C, R, RetE); } +static bool canThisLHSleakRHS(Expr *LHS); +static llvm::DenseSet doesThisRHSContainStackAddr(Expr *RHS); + +void StackAddrEscapeChecker::checkAssignmentStmtForStackObjEscapeToPtrOutParam( + const BinaryOperator *Assignment, CheckerContext &C) const { + if (canThisLHSleakRHS(Assignment->getLHS())) { + llvm::DenseSet AddrsLeaked = + doesThisRHSContainStackAddr(Assignment->getRHS()); + for (VarDecl *Var : AddrsLeaked) { + EmitStackError(C, C.getState()->getRegion(Var, C.getLocationContext()), + Assignment->getRHS()); + } + } +} + +// Returns dereference count for every non-local-address in the expression tree. +// +// Total number of times a sub-(sub-sub-...)expression is dereference. +// dereference (*) ==> +1 +// address-of (&) ==> -1 +// +// The reason why this is tree-traversal are: +// - ArraySubscriptExpr +// - BinaryOperator +// +// We check both LHS/RHS of ArraySubscriptExpr because array[4] <==> 4[array] +// For known false negatives please read comments in the implementation below. +static llvm::DenseSet +dereferenceCountsForNonLocalAddresses(Expr *SubExpr) { + + int DereferenceCount = 0; + + while (SubExpr) { + // FIXME: We are ignoring also user-defined casts (c-tor, conversion + // operator) that can technically be benign even if we pass a stack address + // to them. + SubExpr = SubExpr->IgnoreParenCasts(); + + UnaryOperator *LHSUnOp = dyn_cast(SubExpr); + if (LHSUnOp) { + if (LHSUnOp->getOpcode() == UO_Deref) { + ++DereferenceCount; + SubExpr = LHSUnOp->getSubExpr(); + continue; + } + + if (LHSUnOp->getOpcode() == UO_AddrOf) { + --DereferenceCount; + SubExpr = LHSUnOp->getSubExpr(); + continue; + } + + break; + } + + auto HandlePseudoBinOp = [](Expr *LHS, Expr *RHS, int DereferenceCount) { + llvm::DenseSet Result; + llvm::DenseSet PartResult = + dereferenceCountsForNonLocalAddresses(LHS); + for (const int e : PartResult) { + Result.insert(e + DereferenceCount); + } + PartResult = dereferenceCountsForNonLocalAddresses(RHS); + for (const int e : PartResult) { + Result.insert(e + DereferenceCount); + } + return Result; + }; + + ArraySubscriptExpr *ArraySubscExp = dyn_cast(SubExpr); + if (ArraySubscExp) { + ++DereferenceCount; // because foo[i] <==> *(foo + i) + return HandlePseudoBinOp(ArraySubscExp->getLHS(), ArraySubscExp->getRHS(), + DereferenceCount); + } + + BinaryOperator *BinOp = dyn_cast(SubExpr); + if (BinOp) { + return HandlePseudoBinOp(BinOp->getLHS(), BinOp->getRHS(), + DereferenceCount); + } + + DeclRefExpr *DRef = dyn_cast(SubExpr); + if (DRef) { + NamedDecl *ReferredDecl = DRef->getFoundDecl(); + VarDecl *VD = dyn_cast(ReferredDecl); + if (VD) { + if (VD->isLocalVarDecl()) + return {}; + + // This is specific for LHS - references behave just like objects on + // RHS. + const Type *VType = VD->getType().getTypePtrOrNull(); + if (VType && VType->isReferenceType()) + ++DereferenceCount; + } + llvm::DenseSet Result; + Result.insert(DereferenceCount); + return Result; + } + + MemberExpr *ME = dyn_cast(SubExpr); + if (ME) { + if (ME->isArrow()) { + // because foo->bar <=> (*foo).bar + ++DereferenceCount; + ; + } + SubExpr = ME->getBase(); + continue; + } + + // FIXME: We might get false positives here for these cases + // (non-exhaustive): + // - static methods of local types + // - local lambda objects + // - functions that derive return value from objects in current stack frame + // - functions that derive return value from stack state (e. g. returning + // the stack ptr value) + if (isa(SubExpr)) { + llvm::DenseSet Result; + Result.insert(DereferenceCount); + return Result; + } + + // FIXME: In theory constant addresses are also sort of "global" but we + // assume that they are part of completely benign expressions way more often + // and we don't have a good way to distinguish those from the bad ones. + // Example: + // local_array[0] + break; + } + + return {}; +} + +// Returns dereference count for every local variable address in the expression +// tree. +// +// Total number of times a sub-(sub-sub-...)expression is dereference. +// dereference (*) ==> +1 +// address-of (&) ==> -1 +// +// The reason why this is tree-traversal are: +// - ArraySubscriptExpr +// - BinaryOperator +// +// We check both LHS/RHS of ArraySubscriptExpr because array[4] <==> 4[array] +// For known false negatives please read comments in the implementation below. +static llvm::DenseMap> +dereferenceCountsForLocalAddresses(Expr *SubExpr) { + + int DereferenceCount = 0; + + while (SubExpr) { + // FIXME: We are ignoring also user-defined casts (c-tor, conversion + // operator) that can technically be benign even if we pass a stack address + // to them. + SubExpr = SubExpr->IgnoreParenCasts(); + + UnaryOperator *LHSUnOp = dyn_cast(SubExpr); + if (LHSUnOp) { + if (LHSUnOp->getOpcode() == UO_Deref) { + ++DereferenceCount; + SubExpr = LHSUnOp->getSubExpr(); + continue; + } + + if (LHSUnOp->getOpcode() == UO_AddrOf) { + --DereferenceCount; + SubExpr = LHSUnOp->getSubExpr(); + continue; + } + + break; + } + + auto HandlePseudoBinOp = [](Expr *LHS, Expr *RHS, int DereferenceCount) { + llvm::DenseMap> LHSResult = + dereferenceCountsForLocalAddresses(LHS); + llvm::DenseMap> RHSResult = + dereferenceCountsForLocalAddresses(RHS); + // merge the results of both sub-expressions + for (auto &e : RHSResult) { + auto &BucketInLHSResult = LHSResult[e.first]; + BucketInLHSResult.insert(e.second.begin(), e.second.end()); + } + llvm::DenseMap> Result; + for (auto &e : LHSResult) + Result[e.first + DereferenceCount].insert(e.second.begin(), + e.second.end()); + for (auto &e : RHSResult) + Result[e.first + DereferenceCount].insert(e.second.begin(), + e.second.end()); + + return Result; + }; + + ArraySubscriptExpr *ArraySubscExp = dyn_cast(SubExpr); + if (ArraySubscExp) { + ++DereferenceCount; // because foo[i] <==> *(foo + i) + return HandlePseudoBinOp(ArraySubscExp->getLHS(), ArraySubscExp->getRHS(), + DereferenceCount); + } + + BinaryOperator *BinOp = dyn_cast(SubExpr); + if (BinOp) { + return HandlePseudoBinOp(BinOp->getLHS(), BinOp->getRHS(), + DereferenceCount); + } + + DeclRefExpr *DRef = dyn_cast(SubExpr); + if (DRef) { + NamedDecl *ReferredDecl = DRef->getFoundDecl(); + VarDecl *VD = dyn_cast(ReferredDecl); + if (VD && VD->isLocalVarDecl()) { + llvm::DenseMap> Result; + Result[DereferenceCount].insert(VD); + return Result; + } + + // Call arguments are local in the sense they live in the same stack frame + // as local variables. + if (VD && isa(VD)) { + llvm::DenseMap> Result; + Result[DereferenceCount].insert(VD); + return Result; + } + + return llvm::DenseMap>{}; + } + + MemberExpr *ME = dyn_cast(SubExpr); + if (ME) { + if (ME->isArrow()) { + // because foo->bar <=> (*foo).bar + ++DereferenceCount; + ; + } + SubExpr = ME->getBase(); + continue; + } + + // FIXME: We might get false negatives here for these cases + // (non-exhaustive): + // - static methods of local types + // - local lambda objects + // - functions that derive return value from objects in current stack frame + // - functions that derive return value from stack state (e. g. returning + // the stack ptr value) + if (isa(SubExpr)) { + return llvm::DenseMap>{}; + } + + break; + } + + return llvm::DenseMap>{}; +} + +// Returns true iff the expr (when used as LHS in an assignment) can potentially +// leak the RHS value. We are basically checking if any subexpression contains a +// memory address. If we write any stack address to a pointer (order 1 or +// higher) that was passed as an argument to the function call then the exact +// same value can be used outside of the function body to access the stack +// address. +/* Examples: +param_type assignment OK? +--------------------------------------------------- +unsigned long * (cast) foo = &stack_obj bad +unsigned long ** (cast) foo = &stack_obj bad +unsigned long *& foo = &stack_obj ok + +unsigned long * *foo = &stack_obj bad +unsigned long ** *foo = &stack_obj bad +unsigned long ** **foo = &stack_obj bad +unsigned long ** foo = &stack_obj ok +unsigned long ** &*foo = &stack_obj ok +*/ +static bool canThisLHSleakRHS(Expr *LHS) { + llvm::DenseSet nonlocalDerefCounts = + dereferenceCountsForNonLocalAddresses(LHS); + for (int e : nonlocalDerefCounts) { + if (e > 0) + return true; + } + return false; +} + +// Returns the set of stack-allocated variables (local vars, function params) in +// the \p RHS whose address is published. If there are no such variables the set +// is empty. +// FIXME: We produce false negatives for alloca()-ed objects. +static llvm::DenseSet doesThisRHSContainStackAddr(Expr *RHS) { + llvm::DenseMap> localDerefCounts = + dereferenceCountsForLocalAddresses(RHS); + + llvm::DenseSet Result; + + auto BucketWithAddressesIt = localDerefCounts.find(-1); + if (BucketWithAddressesIt != localDerefCounts.end()) { + Result = BucketWithAddressesIt->second; + } + + // Arrays are represented by address of the first element - in a way there's + // one extra addr-of operator. + BucketWithAddressesIt = localDerefCounts.find(0); + if (BucketWithAddressesIt != localDerefCounts.end()) { + for (VarDecl *VD : BucketWithAddressesIt->second) { + const Type *VType = VD->getType().getTypePtrOrNull(); + if (VType && (isa(VType) || + isa(VType) || + isa(VType))) + Result.insert(VD); + } + } + + return Result; +} + +// FIXME: We produce false negatives for multiple assignments & casts. +// void leaky_function(unsigned long* outparam) { +// int stack_obj; +// unsigned long stack_addr_val; +// stack_addr_val = (unsigned long)&stack_obj; +// *outparam = stack_addr_val; +// } + +// FIXME: We produce false positives when outparams are used as a scratchpad and +// fixed before the function returns. void not_leaky_function(unsigned long* +// scratchpad) { +// int stack_obj; +// scratchpad = (unsigned long)&stack_obj; +// // do stuff ... +// scratchpad = NULL; +// } + +void StackAddrEscapeChecker::checkPreStmt(const Stmt *S, + CheckerContext &C) const { + if (isa(S)) { + checkPreReturnStmt(cast(S), C); + return; + } + if (!ChecksEnabled[CK_StackAddrEscapeChecker]) + return; + + const BinaryOperator *BO = dyn_cast_or_null(S); + if (!BO) + return; + + if (!BO->isAssignmentOp()) + return; + checkAssignmentStmtForStackObjEscapeToPtrOutParam(BO, C); +} + void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, CheckerContext &Ctx) const { if (!ChecksEnabled[CK_StackAddrEscapeChecker]) Index: clang/test/Analysis/Checkers/StackAddrEscapeChecker.c =================================================================== --- /dev/null +++ clang/test/Analysis/Checkers/StackAddrEscapeChecker.c @@ -0,0 +1,91 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core.StackAddressEscape -verify %s + +void leaks_stack_obj_to_outparam(int **outparam) { + int stack_allocated_obj; + *outparam = &stack_allocated_obj; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_obj' returned to caller [core.StackAddressEscape]}} +} + +void leaks_stack_obj_to_outparam_2(int **outparam) { + int stack_allocated_obj; + outparam[0] = &stack_allocated_obj; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_obj' returned to caller [core.StackAddressEscape]}} +} + +void leaks_stack_array_to_outparam(int **outparam) { + int stack_allocated_array[1]; + *outparam = stack_allocated_array; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_array' returned to caller [core.StackAddressEscape]}} +} + +void leaks_stack_obj_to_outparam_array(int *outparam[1]) { + int stack_allocated_obj; + outparam[0] = &stack_allocated_obj; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_obj' returned to caller [core.StackAddressEscape]}} +} + +void leaks_stack_obj_to_outparam_many_unop_LHS(int **outparam) { + int stack_allocated_array; + *&*outparam = &stack_allocated_array; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_array' returned to caller [core.StackAddressEscape]}} +} + +void leaks_stack_obj_to_outparam_many_unop_RHS(int **outparam) { + int stack_allocated_array; + *outparam = &*&stack_allocated_array; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_array' returned to caller [core.StackAddressEscape]}} +} + +struct foo_s { + int *stuff; +}; + +void leaks_stack_obj_to_outparam_struct(struct foo_s *outparam) { + int stack_allocated_obj; + outparam->stuff = &stack_allocated_obj; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_obj' returned to caller [core.StackAddressEscape]}} +} + +void leaks_call_arg_to_outparam(int param, int **outparam) { + *outparam = ¶m; // expected-warning{{Address of stack memory associated with local variable 'param' returned to caller [core.StackAddressEscape]}} +} + +void dont_leak_int(void) { + int ** local_ptr_ptr; + int local_var; + // This is OK as nothing escapes the stack frame. + *local_ptr_ptr = &local_var; +} + +void dont_leak_struct(void) { + struct foo_s local_struct; + int local_var; + // This is OK as nothing escapes the stack frame. + local_struct.stuff = &local_var; +} + +void assigns_param_to_param(int** outparam, int* inparam) { + // We don't know the lifetime of inparam - the caller is managing it. + *outparam = inparam; +} + +void assigns_param_to_param_structs(struct foo_s* outparam, struct foo_s* inparam) { + // We don't know the lifetime of inparam - the caller is managing it. + outparam->stuff = inparam->stuff; +} + +void assigns_ptr_to_outparam(int ** outparam) { + int* local_var; + // FIXME: The simple AST-based approach can't reason about the value so it's inconclusive if this is OK or not. + // OK example: + // int* local_var = NULL; + // OK example: + // int* local_var = malloc(sizeof(int)); + // BAD example: + // int a; int* local_var = &a; + // BAD example: + // int* local_var = alloca(sizeof(int)); + *outparam = local_var; +} + +// TODO - is this too invasive or is it an incorrect code in 100% cases? +int** return_random_ptr_ptr(void); +void leaks_stack_obj_to_function_return_value(struct foo_s *outparam) { + int stack_allocated_obj; + *(return_random_ptr_ptr()) = &stack_allocated_obj; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_obj' returned to caller [core.StackAddressEscape]}} +} + +// TODO assigning to a global Index: clang/test/Analysis/Checkers/StackAddrEscapeChecker.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/Checkers/StackAddrEscapeChecker.cpp @@ -0,0 +1,25 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core.StackAddressEscape -verify %s + +void leaks_stack_obj_to_outparam(unsigned long& outparam) { + int stack_allocated_obj = 42; + outparam = (unsigned long) &stack_allocated_obj; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_obj' returned to caller [core.StackAddressEscape]}} +} + +void dont_leak_stack_obj_to_outparam(int& outparam) { + int stack_allocated_obj = 42; + outparam = stack_allocated_obj; +} + +void leaks_stack_obj_to_outparam_2(int*& outparam) { + int stack_allocated_obj; + outparam = &stack_allocated_obj; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_obj' returned to caller [core.StackAddressEscape]}} +} + +struct foo_s { + int *stuff; +}; + +void leaks_stack_obj_to_outparam_struct(foo_s& outparam) { + int stack_allocated_obj; + outparam.stuff = &stack_allocated_obj; // expected-warning{{Address of stack memory associated with local variable 'stack_allocated_obj' returned to caller [core.StackAddressEscape]}} +} Index: clang/test/Analysis/loop-block-counts.c =================================================================== --- clang/test/Analysis/loop-block-counts.c +++ clang/test/Analysis/loop-block-counts.c @@ -4,7 +4,7 @@ void callee(void **p) { int x; - *p = &x; + *p = &x; // // expected-warning{{Address of stack memory associated with local variable 'x' returned to caller [core.StackAddressEscape]}} } void loop() {