From 6e7650dd0d17925d23c2a75da32c872f5dc40b42 Mon Sep 17 00:00:00 2001 From: George Karpenkov Date: Mon, 14 May 2018 21:39:54 +0000 Subject: [PATCH] [analyzer] Extend the ObjCAutoreleaseWriteChecker to warn on captures as well A common pattern is that the code in the block does not write into the variable explicitly, but instead passes it to a helper function which performs the write. Differential Revision: https://reviews.llvm.org/D46772 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@332300 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/ObjCAutoreleaseWriteChecker.cpp | 80 ++++++++++++------- test/Analysis/autoreleasewritechecker_test.m | 55 +++++++++++++ 2 files changed, 106 insertions(+), 29 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp index 3e85256b53..5199959dee 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp @@ -8,7 +8,7 @@ //===----------------------------------------------------------------------===// // // This file defines ObjCAutoreleaseWriteChecker which warns against writes -// into autoreleased out parameters which are likely to cause crashes. +// into autoreleased out parameters which cause crashes. // An example of a problematic write is a write to {@code error} in the example // below: // @@ -21,8 +21,9 @@ // return false; // } // -// Such code is very likely to crash due to the other queue autorelease pool -// begin able to free the error object. +// Such code will crash on read from `*error` due to the autorelease pool +// in `enumerateObjectsUsingBlock` implementation freeing the error object +// on exit from the function. // //===----------------------------------------------------------------------===// @@ -41,8 +42,9 @@ using namespace ast_matchers; namespace { const char *ProblematicWriteBind = "problematicwrite"; +const char *CapturedBind = "capturedbind"; const char *ParamBind = "parambind"; -const char *MethodBind = "methodbind"; +const char *IsMethodBind = "ismethodbind"; class ObjCAutoreleaseWriteChecker : public Checker { public: @@ -110,67 +112,87 @@ static void emitDiagnostics(BoundNodes &Match, const Decl *D, BugReporter &BR, AnalysisDeclContext *ADC = AM.getAnalysisDeclContext(D); const auto *PVD = Match.getNodeAs(ParamBind); - assert(PVD); QualType Ty = PVD->getType(); if (Ty->getPointeeType().getObjCLifetime() != Qualifiers::OCL_Autoreleasing) return; - const auto *SW = Match.getNodeAs(ProblematicWriteBind); - bool IsMethod = Match.getNodeAs(MethodBind) != nullptr; + const char *WarningMsg = "Write to"; + const auto *MarkedStmt = Match.getNodeAs(ProblematicWriteBind); + + // Prefer to warn on write, but if not available, warn on capture. + if (!MarkedStmt) { + MarkedStmt = Match.getNodeAs(CapturedBind); + assert(MarkedStmt); + WarningMsg = "Capture of"; + } + + SourceRange Range = MarkedStmt->getSourceRange(); + PathDiagnosticLocation Location = PathDiagnosticLocation::createBegin( + MarkedStmt, BR.getSourceManager(), ADC); + bool IsMethod = Match.getNodeAs(IsMethodBind) != nullptr; const char *Name = IsMethod ? "method" : "function"; - assert(SW); BR.EmitBasicReport( ADC->getDecl(), Checker, - /*Name=*/"Write to autoreleasing out parameter inside autorelease pool", + /*Name=*/(llvm::Twine(WarningMsg) + + " autoreleasing out parameter inside autorelease pool").str(), /*Category=*/"Memory", - (llvm::Twine("Write to autoreleasing out parameter inside ") + + (llvm::Twine(WarningMsg) + " autoreleasing out parameter inside " + "autorelease pool that may exit before " + Name + " returns; consider " "writing first to a strong local variable declared outside of the block") .str(), - PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC), - SW->getSourceRange()); + Location, + Range); } void ObjCAutoreleaseWriteChecker::checkASTCodeBody(const Decl *D, AnalysisManager &AM, BugReporter &BR) const { + auto DoublePointerParamM = + parmVarDecl(hasType(hasCanonicalType(pointerType( + pointee(hasCanonicalType(objcObjectPointerType())))))) + .bind(ParamBind); + + auto ReferencedParamM = + declRefExpr(to(parmVarDecl(DoublePointerParamM))); + // Write into a binded object, e.g. *ParamBind = X. auto WritesIntoM = binaryOperator( hasLHS(unaryOperator( hasOperatorName("*"), hasUnaryOperand( - ignoringParenImpCasts( - declRefExpr(to(parmVarDecl(equalsBoundNode(ParamBind)))))) + ignoringParenImpCasts(ReferencedParamM)) )), hasOperatorName("=") ).bind(ProblematicWriteBind); + auto ArgumentCaptureM = hasAnyArgument( + ignoringParenImpCasts(ReferencedParamM)); + auto CapturedInParamM = stmt(anyOf( + callExpr(ArgumentCaptureM), + objcMessageExpr(ArgumentCaptureM))).bind(CapturedBind); + // WritesIntoM happens inside a block passed as an argument. - auto WritesInBlockM = hasAnyArgument(allOf( + auto WritesOrCapturesInBlockM = hasAnyArgument(allOf( hasType(hasCanonicalType(blockPointerType())), - forEachDescendant(WritesIntoM) - )); + forEachDescendant( + stmt(anyOf(WritesIntoM, CapturedInParamM)) + ))); - auto CallsAsyncM = stmt(anyOf( + auto BlockPassedToMarkedFuncM = stmt(anyOf( callExpr(allOf( - callsNames(FunctionsWithAutoreleasingPool), WritesInBlockM)), + callsNames(FunctionsWithAutoreleasingPool), WritesOrCapturesInBlockM)), objcMessageExpr(allOf( hasAnySelector(toRefs(SelectorsWithAutoreleasingPool)), - WritesInBlockM)) + WritesOrCapturesInBlockM)) )); - auto DoublePointerParamM = - parmVarDecl(hasType(hasCanonicalType(pointerType( - pointee(hasCanonicalType(objcObjectPointerType())))))) - .bind(ParamBind); - - auto HasParamAndWritesAsyncM = allOf( + auto HasParamAndWritesInMarkedFuncM = allOf( hasAnyParameter(DoublePointerParamM), - forEachDescendant(CallsAsyncM)); + forEachDescendant(BlockPassedToMarkedFuncM)); auto MatcherM = decl(anyOf( - objcMethodDecl(HasParamAndWritesAsyncM).bind(MethodBind), - functionDecl(HasParamAndWritesAsyncM))); + objcMethodDecl(HasParamAndWritesInMarkedFuncM).bind(IsMethodBind), + functionDecl(HasParamAndWritesInMarkedFuncM))); auto Matches = match(MatcherM, *D, AM.getASTContext()); for (BoundNodes Match : Matches) diff --git a/test/Analysis/autoreleasewritechecker_test.m b/test/Analysis/autoreleasewritechecker_test.m index 32ba594352..eb79052003 100644 --- a/test/Analysis/autoreleasewritechecker_test.m +++ b/test/Analysis/autoreleasewritechecker_test.m @@ -205,10 +205,65 @@ BOOL writeToErrorWithIterator(NSError *__autoreleasing* error, NSArray *a, NSSet return 0; } +void writeIntoError(NSError **error) { + *error = [NSError errorWithDomain:1]; +} + +extern void readError(NSError *error); + void writeToErrorWithIteratorNonnull(NSError *__autoreleasing* _Nonnull error, NSDictionary *a) { [a enumerateKeysAndObjectsUsingBlock:^{ *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter}} }]; } + + +void escapeErrorFromIterator(NSError *__autoreleasing* _Nonnull error, NSDictionary *a) { + [a enumerateKeysAndObjectsUsingBlock:^{ + writeIntoError(error); // expected-warning{{Capture of autoreleasing out parameter}} + }]; +} + +void noWarningOnRead(NSError *__autoreleasing* error, NSDictionary *a) { + [a enumerateKeysAndObjectsUsingBlock:^{ + NSError* local = *error; // no-warning + }]; +} + +void noWarningOnEscapeRead(NSError *__autoreleasing* error, NSDictionary *a) { + [a enumerateKeysAndObjectsUsingBlock:^{ + readError(*error); // no-warning + }]; +} + +@interface ErrorCapture +- (void) captureErrorOut:(NSError**) error; +- (void) captureError:(NSError*) error; +@end + +void escapeErrorFromIteratorMethod(NSError *__autoreleasing* _Nonnull error, + NSDictionary *a, + ErrorCapture *capturer) { + [a enumerateKeysAndObjectsUsingBlock:^{ + [capturer captureErrorOut:error]; // expected-warning{{Capture of autoreleasing out parameter}} + }]; +} + +void noWarningOnEscapeReadMethod(NSError *__autoreleasing* error, + NSDictionary *a, + ErrorCapture *capturer) { + [a enumerateKeysAndObjectsUsingBlock:^{ + [capturer captureError:*error]; // no-warning + }]; +} + +void multipleErrors(NSError *__autoreleasing* error, NSDictionary *a) { + [a enumerateKeysAndObjectsUsingBlock:^{ + writeIntoError(error); // expected-warning{{Capture of autoreleasing out parameter}} + *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter}} + writeIntoError(error); // expected-warning{{Capture of autoreleasing out parameter}} + }]; +} + #endif -- 2.40.0