Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1410,6 +1410,100 @@ } } +static bool isTrivialCopyOrMoveCtor(const CXXConstructExpr *CE) { + if (!CE) + return false; + + const auto *CtorDecl = CE->getConstructor(); + + return (CtorDecl->isCopyConstructor() || CtorDecl->isMoveConstructor()) && + CtorDecl->isTrivial(); +} + +// Let's assume we have nested list initialization, for example: +// struct S3 { +// int *x, *y; +// }; +// +// struct S2 { +// S3 s3; +// }; +// +// struct S { +// S2 s2; +// +// S(int *x, int *y) : s2{x, y} {}; +// }; +// +// And we have a snippet like the following: +// int *x = nullptr, *y = nullptr; +// S s{x, y}; +// *s.s2.s3.y; +// +// The memory region StoreSiteFinder tracks will be `s.s2.s3.y`, however +// the InitListExpr, we visit will be +// `-InitListExpr 'S2':'S2' +// `-InitListExpr 'S3':'S3' +// |-ImplicitCastExpr 'int *' +// | `-DeclRefExpr 'int *' lvalue ParmVar 'x' 'int *' +// `-ImplicitCastExpr 'int *' +// `-DeclRefExpr 'int *' lvalue ParmVar 'y' 'int *' +// +// We somehow need to use the real initializer expression, which is +// `-ImplicitCastExpr 'int *' +// `-DeclRefExpr 'int *' lvalue ParmVar 'y' 'int *' +// +// The way this function works is that it backtracks from `s.s2.s3.y` +// in the direction of `s`, so it will go like `s.s2.s3.y` -> `s.s2.s3` -> +// `s.s2` -> `s`. +// +// If somewhere the parent memory region matches the `InitListExpr`, that +// we were provided it stops and returns the expression that initialized +// the upcoming field of the region based on it's index. +// +// In this case there will be a match on `s.s2.s3`, because the parent region +// is `s2` and the provided initializer is for `s2`. The index of `s3` inside +// `s2` is 0, so the returned expression will be +// `-InitListExpr 'S3':'S3' +// |-ImplicitCastExpr 'int *' +// | `-DeclRefExpr 'int *' lvalue ParmVar 'x' 'int *' +// `-ImplicitCastExpr 'int *' +// `-DeclRefExpr 'int *' lvalue ParmVar 'y' 'int *' +// +// In the stackframe, where we returned this, the "stored" region is +// `s.s2.s3.y`, so we simply take the index of `y` inside `s3` and we have the +// expression we wanted, which is +// `-ImplicitCastExpr 'int *' +// `-DeclRefExpr 'int *' lvalue ParmVar 'y' 'int *' +// +// FIXME: Pass a tracker and launch for each call to get more accurate notes. +const Expr *recursiveGetNestedInitListInitializer(const Expr *E, + const MemRegion *R) { + if (!isa(R) || !isa(E)) + return nullptr; + + const auto *FR = R->getAs(); + const auto *FD = FR->getDecl(); + const auto *ILE = cast(E); + + // If we can match the region with the current initializer, return the + // initializer. + if (ILE->getType()->getUnqualifiedDesugaredType() == + FD->getParent()->getTypeForDecl()) + return ILE->getInit(FD->getFieldIndex()); + + // Try to find a point, where we can match the initializer with a super + // region. + const auto *match = + recursiveGetNestedInitListInitializer(ILE, FR->getSuperRegion()); + ILE = dyn_cast_or_null(match); + + if (!ILE) + return match; + + return ILE->getInit(FD->getFieldIndex()); +} + PathDiagnosticPieceRef StoreSiteFinder::VisitNode(const ExplodedNode *Succ, BugReporterContext &BRC, PathSensitiveBugReport &BR) { @@ -1458,11 +1552,106 @@ // If this is an assignment expression, we can track the value // being assigned. - if (Optional P = Succ->getLocationAs()) + if (Optional P = Succ->getLocationAs()) { if (const BinaryOperator *BO = P->getStmtAs()) if (BO->isAssignmentOp()) InitE = BO->getRHS(); + // Will happen in cases like S s{x, y}; + if (const auto *DS = P->getStmtAs()) + InitE = cast(DS->getSingleDecl())->getInit(); + + // This case occurs if we have a nested list initialization. + // E.g.: + // + // struct S { + // S2 s2; + // S(int *x, int *y) : s2{x, y} {} + // }; + // + // Note that these can also be nested. + if (const auto *ILE = P->getStmtAs()) + InitE = recursiveGetNestedInitListInitializer(ILE, R); + + // We reached a copy/move constructor that hasn't been inlined (most + // likely because of ExprEngine::performTrivialCopy()). The idea is to + // track the value back to the first constructor call. + // + // Note that the copy/move constructors can be chained, so + // in that case we also track one constructor back to the other one. + // E.g.: + // + // struct S { + // int *a, *b; + // S(int *a, int *b) : p1(a), p2(b) {} + // }; + // + // void foo() { + // int *x = nullptr, *y = nullptr; + // + // S s(x, y); + // S s2 = s; + // S s3 = s2; <-- here we track 's3.p1' all + // the way back to 's.p1' and + // launch a separate tracker + // to see where s2 comes from + // + // int i = *s3.p1; <-- null dereference here + // } + // + if (const auto *CE = dyn_cast_or_null( + Succ->getStmtForDiagnostics())) { + const MemRegion *Reg = R; + const ExplodedNode *N = Succ; + + // If the copy or move constructor is user defined it will be inlined + // and handled elsewhere. + while (isTrivialCopyOrMoveCtor(CE)) { + const auto *FR = dyn_cast(Reg); + + if (!FR) { + N = nullptr; + break; + } + + // Look up the value in the origin object. + auto OriginObject = CE->getArg(0); + auto OriginVal = + N->getState()->getSVal(OriginObject, N->getLocationContext()); + auto OriginFieldVal = + N->getState()->getLValue(FR->getDecl(), OriginVal); + Reg = OriginFieldVal.getAsRegion(); + + // Find the node where the value in the origin object was initialized. + N = N->getFirstPred(); + while (N && N->getState()->getSVal(Reg) == V) + N = N->getFirstPred(); + + if (!N) + break; + + // Track the object, we copied or moved from. + CE = dyn_cast(N->getStmtForDiagnostics()); + getParentTracker().track(OriginVal, OriginVal.getAsRegion(), Options); + } + + if (N) { + auto StmtPt = N->getLocationAs(); + + // if(const auto *ILE = StmtPt->getStmtAs()) + // InitE = ILE->getInit(0); + // else + if (const auto *E = StmtPt->getStmtAs()) + InitE = E; + else if (const auto *DS = StmtPt->getStmtAs()) { + const auto *VD = cast(DS->getSingleDecl()); + + InitE = VD->getInit(); + } + } + } + } + // If this is a call entry, the variable should be a parameter. // FIXME: Handle CXXThisRegion as well. (This is not a priority because // 'this' should never be NULL, but this visitor isn't just for NULL and @@ -1504,6 +1693,28 @@ if (!IsParam) InitE = InitE->IgnoreParenCasts(); + // An initializer list has been used to initialize an object + // E.g.: Obj o{ 1, 2 }; + // + // The idea here is to match the field index with the initializer + // index to find out which expression to track. + if (isa(R) && isa(InitE)) { + const auto *FR = R->getAs(); + const auto *FD = FR->getDecl(); + const auto *ILE = cast(InitE); + + // The type of the initializer list matches the type of the object, which + // means the object was list initialized. E.g.: Obj o{ 1, 2 }. + if (InitE->getType()->getUnqualifiedDesugaredType() == + FD->getParent()->getTypeForDecl()) + InitE = ILE->getInit(FR->getDecl()->getFieldIndex()); + + // We hit some other kind of initializer. E.g.: Contrustor() : + // field{*here*} {} + else + InitE = ILE->getInit(0); + } + getParentTracker().track(InitE, StoreSite, Options); } @@ -2418,12 +2629,12 @@ track(BO->getLHS()); if (RHSV.isZeroConstant()) track(BO->getRHS()); - } else { // Track only the LHS of a division or a modulo. - if (LHSV.isZeroConstant()) - track(BO->getLHS()); - } + } else { // Track only the LHS of a division or a modulo. + if (LHSV.isZeroConstant()) + track(BO->getLHS()); + } - return CombinedResult; + return CombinedResult; } }; Index: clang/test/Analysis/ctor-bug-path.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/ctor-bug-path.cpp @@ -0,0 +1,115 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-output=text -verify %s + +#include "Inputs/system-header-simulator-cxx.h" + +namespace copyMoveTrackCtor { +struct S { + int *p1, *p2; + S(int *a, int *b) : p1(a), p2(b) {} +}; + +void CtorDirect() { + int *x = nullptr, *y = nullptr; //expected-note{{'x' initialized to a null pointer value}} + + S s(x, y); //expected-note{{'s' initialized here}} + // expected-note@-1{{Passing null pointer value via 1st parameter 'a'}} + S s2 = s; //expected-note{{'s2' initialized here}} + S s3 = s2; //expected-note{{'s3' initialized here}} + S s4 = std::move(s3); //expected-note{{'s4' initialized here}} + S s5 = s4; // expected-note{{Null pointer value stored to 's5.p1'}} + + int i = *s5.p1; // expected-warning{{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} + + (void) i; +} +} // namespace copyMoveTrackCtor + +namespace copyMoveTrackInitList { +struct S { + int *p1, *p2; +}; + +void InitListDirect() { + int *x = nullptr, *y = nullptr; //expected-note{{'x' initialized to a null pointer value}} + + S s{x, y}; //expected-note{{'s' initialized here}} + S s2 = s; //expected-note{{'s2' initialized here}} + S s3 = s2; //expected-note{{'s3' initialized here}} + S s4 = std::move(s3); //expected-note{{'s4' initialized here}} + S s5 = s4; // expected-note{{Null pointer value stored to 's5.p1'}} + + int i = *s5.p1; // expected-warning{{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} + + (void) i; +} + +void InitListAssign() { + int *x = nullptr, *y = nullptr; //expected-note{{'x' initialized to a null pointer value}} + + S s = {x, y}; //expected-note{{'s' initialized here}} + S s2 = s; //expected-note{{'s2' initialized here}} + S s3 = s2; //expected-note{{'s3' initialized here}} + S s4 = std::move(s3); //expected-note{{'s4' initialized here}} + S s5 = s4; // expected-note{{Null pointer value stored to 's5.p1'}} + + int i = *s5.p1; // expected-warning{{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} + + (void) i; +} + +} // namespace copyMoveTrackInitList + +namespace directInitList { +struct S { + int *p1, *p2; +}; + +void InitListDirect() { + int *x = nullptr, *y = nullptr; //expected-note{{'y' initialized to a null pointer value}} + + S s{x, y}; //expected-note{{'s.p2' initialized to a null pointer value}} + + int i = *s.p2; // expected-warning{{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} + (void) i; +} +} // namespace directInitList + +namespace nestedCtorInitializer { + struct S5{ + int *x, *y; + }; + + struct S4 { + S5 s5; + }; + + struct S3 { + S4 s4; + }; + + struct S2 { + S3 s3; + }; + + struct S { + S2 s2; + + S(int *x, int *y) : s2{x, y} {}; // expected-note{{Null pointer value stored to 's.s2.s3.s4.s5.y'}} + }; + + void nestedCtorInit(){ + int *x = nullptr, *y = nullptr; //expected-note{{'y' initialized to a null pointer value}} + + S s{x,y}; // expected-note{{Calling constructor for 'S'}} + // expected-note@-1{{Returning from constructor for 'S'}} + // expected-note@-2{{Passing null pointer value via 2nd parameter}} + + int i = *s.s2.s3.s4.s5.y;// expected-warning{{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} + (void) i; + } +} // namespace nestedCtorInitializer