Index: lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp +++ 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 @@ 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 @@ 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(equalsBoundNode(ParamBind)))); + // 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) Index: test/Analysis/autoreleasewritechecker_test.m =================================================================== --- test/Analysis/autoreleasewritechecker_test.m +++ test/Analysis/autoreleasewritechecker_test.m @@ -205,10 +205,65 @@ 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