Index: clang/include/clang/Basic/Attr.td =================================================================== --- clang/include/clang/Basic/Attr.td +++ clang/include/clang/Basic/Attr.td @@ -840,13 +840,13 @@ def OSReturnsRetained : InheritableAttr { let Spellings = [Clang<"os_returns_retained">]; - let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>; + let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty, ParmVar]>; let Documentation = [RetainBehaviorDocs]; } def OSReturnsNotRetained : InheritableAttr { let Spellings = [Clang<"os_returns_not_retained">]; - let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>; + let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty, ParmVar]>; let Documentation = [RetainBehaviorDocs]; } Index: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp @@ -529,38 +529,108 @@ C.addTransition(state); } -static ProgramStateRef updateOutParameter(ProgramStateRef State, - SVal ArgVal, - ArgEffectKind Effect) { - auto *ArgRegion = dyn_cast_or_null(ArgVal.getAsRegion()); - if (!ArgRegion) - return State; - - QualType PointeeTy = ArgRegion->getValueType(); - if (!coreFoundation::isCFObjectRef(PointeeTy)) - return State; - - SVal PointeeVal = State->getSVal(ArgRegion); - SymbolRef Pointee = PointeeVal.getAsLocSymbol(); - if (!Pointee) - return State; - - switch (Effect) { - case UnretainedOutParameter: - State = setRefBinding(State, Pointee, - RefVal::makeNotOwned(ObjKind::CF, PointeeTy)); - break; - case RetainedOutParameter: - // Do nothing. Retained out parameters will either point to a +1 reference - // or NULL, but the way you check for failure differs depending on the API. - // Consequently, we don't have a good way to track them yet. - break; +/// \return Whether the chain of typedefs starting from {@code QT} +/// has a typedef with a given name {@code Name}. +static bool hasTypedefNamed(QualType QT, + StringRef Name) { + while (auto *T = dyn_cast(QT)) { + const auto &Context = T->getDecl()->getASTContext(); + if (T->getDecl()->getIdentifier() == &Context.Idents.get(Name)) + return true; + QT = T->getDecl()->getUnderlyingType(); + } + return false; +} + +static SmallVector +updateOutParameter(ProgramStateRef State, + const RetainSummary &Summ, + const CallEvent &CE) { + SVal L = CE.getReturnValue(); + + // Splitting is required to support out parameters, + // as out parameters might be created only on the "success" branch. + // We want to avoid eagerly splitting unless out parameters are actually + // needed. + // So we do calculations on three states: original and two split ones, + // and if no split was necessary return the original one, + // and return the split ones otherwise. + bool SplitNecessary = false; + + ProgramStateRef AssumeTrue = State; + ProgramStateRef AssumeFalse = State; + QualType QT = CE.getResultType(); + bool ShouldSplit = !QT.isNull() && !QT->isVoidType(); + + // The usual convention is for non-zero to mean success, but in the kernel + // it's reverted if the typedef chain has a typedef kern_return_t. + bool SuccessInverted = hasTypedefNamed(QT, "kern_return_t"); + if (!QT.isNull()) { + if (auto DL = L.getAs()) { + AssumeTrue = AssumeTrue->assume(*DL, true); + AssumeFalse = AssumeFalse->assume(*DL, false); + } + } + + for (unsigned idx = 0, e = CE.getNumArgs(); idx != e; ++idx) { + SVal ArgVal = CE.getArgSVal(idx); + ArgEffect AE = Summ.getArg(idx); + + auto *ArgRegion = dyn_cast_or_null(ArgVal.getAsRegion()); + if (!ArgRegion) + continue; + + QualType PointeeTy = ArgRegion->getValueType(); + SVal PointeeVal = State->getSVal(ArgRegion); + SymbolRef Pointee = PointeeVal.getAsLocSymbol(); + if (!Pointee) + continue; + + switch (AE.getKind()) { + case UnretainedOutParameter: { + auto updateOutParam = [&](ProgramStateRef St) { + return setRefBinding(St, Pointee, + RefVal::makeNotOwned(AE.getObjKind(), PointeeTy)); + }; + State = updateOutParam(State); + AssumeTrue = updateOutParam(AssumeTrue); + AssumeFalse = updateOutParam(AssumeFalse); + break; + } + case RetainedOutParameter: { + if (AE.getObjKind() == ObjKind::OS) { + auto updateOutParam = [&](ProgramStateRef St) { + return setRefBinding(St, Pointee, + RefVal::makeOwned(ObjKind::OS, PointeeTy)); + }; + if (!ShouldSplit) { + State = updateOutParam(State); + } else if (!SuccessInverted) { + SplitNecessary = true; + AssumeTrue = updateOutParam(AssumeTrue); + } else { + SplitNecessary = true; + AssumeFalse = updateOutParam(AssumeFalse); + } + } - default: - llvm_unreachable("only for out parameters"); + // For others: + // Do nothing. Retained out parameters will either point to a +1 reference + // or NULL, but the way you check for failure differs depending on the + // API. Consequently, we don't have a good way to track them yet. + break; + } + default: + llvm_unreachable("Only for out parameters"); + } } - return State; + if (SplitNecessary) { + assert(ShouldSplit); + return {AssumeTrue, AssumeFalse}; + } else { + return {State}; + } } void RetainCountChecker::checkSummary(const RetainSummary &Summ, @@ -582,10 +652,7 @@ SVal V = CallOrMsg.getArgSVal(idx); ArgEffect Effect = Summ.getArg(idx); - if (Effect.getKind() == RetainedOutParameter || - Effect.getKind() == UnretainedOutParameter) { - state = updateOutParameter(state, V, Effect.getKind()); - } else if (SymbolRef Sym = V.getAsLocSymbol()) { + if (SymbolRef Sym = V.getAsLocSymbol()) { if (const RefVal *T = getRefBinding(state, Sym)) { if (shouldEscapeOSArgumentOnCall(CallOrMsg, idx, T)) @@ -661,10 +728,15 @@ state = setRefBinding(state, Sym, *updatedRefVal); } - if (DeallocSent) { - C.addTransition(state, C.getPredecessor(), &DeallocSentTag); - } else { - C.addTransition(state); + SmallVector Out = + updateOutParameter(state, Summ, CallOrMsg); + + for (ProgramStateRef St : Out) { + if (DeallocSent) { + C.addTransition(St, C.getPredecessor(), &DeallocSentTag); + } else { + C.addTransition(St); + } } } Index: clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp +++ clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp @@ -822,10 +822,13 @@ return true; } else if (auto K = hasAnyEnabledAttrOf(pd, QT)) { Template->addArg(AF, parm_idx, ArgEffect(RetainedOutParameter, *K)); return true; - } else if (auto K = hasAnyEnabledAttrOf(pd, QT)) { + } else if (auto K = hasAnyEnabledAttrOf(pd, QT)) { Template->addArg(AF, parm_idx, ArgEffect(UnretainedOutParameter, *K)); return true; } else { Index: clang/test/Analysis/osobject-retain-release.cpp =================================================================== --- clang/test/Analysis/osobject-retain-release.cpp +++ clang/test/Analysis/osobject-retain-release.cpp @@ -94,6 +94,134 @@ void escape_with_source(void *p) {} bool coin(); +bool write_into_out_param_on_success(OS_RETURNS_RETAINED OSObject **obj); + +void use_out_param() { + OSObject *obj; + if (write_into_out_param_on_success(&obj)) { + obj->release(); + } +} + +void use_out_param_leak() { + OSObject *obj; + write_into_out_param_on_success(&obj); // expected-note{{Call to function 'write_into_out_param_on_success' returns an OSObject of type OSObject with a +1 retain count}} +} // expected-warning{{Potential leak of an object stored into 'obj'}} + // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}} + +typedef int kern_return_t; +typedef kern_return_t IOReturn; +typedef kern_return_t OSReturn; +#define kOSReturnSuccess 0 +#define kIOReturnSuccess 0 + +bool write_into_two_out_params(OS_RETURNS_RETAINED OSObject **a, + OS_RETURNS_RETAINED OSObject **b); + +void use_write_into_two_out_params() { + OSObject *obj1; + OSObject *obj2; + if (write_into_two_out_params(&obj1, &obj2)) { + obj1->release(); + obj2->release(); + } +} + +void use_write_two_out_params_leak() { + OSObject *obj1; + OSObject *obj2; + write_into_two_out_params(&obj1, &obj2); // expected-note{{Call to function 'write_into_two_out_params' returns an OSObject of type OSObject with a +1 retain count}} + // expected-note@-1{{Call to function 'write_into_two_out_params' returns an OSObject of type OSObject with a +1 retain count}} +} // expected-warning{{Potential leak of an object stored into 'obj1'}} + // expected-warning@-1{{Potential leak of an object stored into 'obj2'}} + // expected-note@-2{{Object leaked: object allocated and stored into 'obj1' is not referenced later in this execution path and has a retain count of +1}} + // expected-note@-3{{Object leaked: object allocated and stored into 'obj2' is not referenced later in this execution path and has a retain count of +1}} + +void always_write_into_two_out_params(OS_RETURNS_RETAINED OSObject **a, + OS_RETURNS_RETAINED OSObject **b); + +void use_always_write_into_two_out_params() { + OSObject *obj1; + OSObject *obj2; + always_write_into_two_out_params(&obj1, &obj2); + obj1->release(); + obj2->release(); +} + +void use_always_write_into_two_out_params_leak() { + OSObject *obj1; + OSObject *obj2; + always_write_into_two_out_params(&obj1, &obj2); // expected-note{{Call to function 'always_write_into_two_out_params' returns an OSObject of type OSObject with a +1 retain count}} + // expected-note@-1{{Call to function 'always_write_into_two_out_params' returns an OSObject of type OSObject with a +1 retain count}} +} // expected-warning{{Potential leak of an object stored into 'obj1'}} + // expected-warning@-1{{Potential leak of an object stored into 'obj2'}} + // expected-note@-2{{Object leaked: object allocated and stored into 'obj1' is not referenced later in this execution path and has a retain count of +1}} + // expected-note@-3{{Object leaked: object allocated and stored into 'obj2' is not referenced later in this execution path and has a retain count of +1}} + +char *write_into_out_param_on_nonnull(OS_RETURNS_RETAINED OSObject **obj); + +void use_out_param_osreturn_on_nonnull() { + OSObject *obj; + if (write_into_out_param_on_nonnull(&obj)) { + obj->release(); + } +} + +void use_out_param_leak_osreturn_on_nonnull() { + OSObject *obj; + write_into_out_param_on_nonnull(&obj); // expected-note{{Call to function 'write_into_out_param_on_nonnull' returns an OSObject of type OSObject with a +1 retain count}} +} // expected-warning{{Potential leak of an object stored into 'obj'}} + // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}} + +bool write_optional_out_param(OS_RETURNS_RETAINED OSObject **obj=nullptr); + +void use_optional_out_param() { + if (write_optional_out_param()) {}; +} + +OSReturn write_into_out_param_on_os_success(OS_RETURNS_RETAINED OSObject **obj); + +void write_into_non_retained_out_param(OS_RETURNS_NOT_RETAINED OSObject **obj); + +void use_write_into_non_retained_out_param() { + OSObject *obj; + write_into_non_retained_out_param(&obj); +} + +void use_write_into_non_retained_out_param_uaf() { + OSObject *obj; + write_into_non_retained_out_param(&obj); // expected-note{{Call to function 'write_into_non_retained_out_param' returns an OSObject of type OSObject with a +0 retain count}} + obj->release(); // expected-warning{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}} + // expected-note@-1{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}} +} + +void always_write_into_out_param(OS_RETURNS_RETAINED OSObject **obj); + +void use_void_out_param_osreturn() { + OSObject *obj; + always_write_into_out_param(&obj); + obj->release(); +} + +void use_void_out_param_osreturn_leak() { + OSObject *obj; + always_write_into_out_param(&obj); // expected-note{{Call to function 'always_write_into_out_param' returns an OSObject of type OSObject with a +1 retain count}} +} // expected-warning{{Potential leak of an object stored into 'obj'}} + // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}} + +void use_out_param_osreturn() { + OSObject *obj; + if (write_into_out_param_on_os_success(&obj) == kOSReturnSuccess) { + obj->release(); + } +} + +void use_out_param_leak_osreturn() { + OSObject *obj; + write_into_out_param_on_os_success(&obj); // expected-note{{Call to function 'write_into_out_param_on_os_success' returns an OSObject of type OSObject with a +1 retain count}} +} // expected-warning{{Potential leak of an object stored into 'obj'}} + // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}} + bool os_consume_violation_two_args(OS_CONSUME OSObject *obj, bool extra) { if (coin()) { // expected-note{{Assuming the condition is false}} // expected-note@-1{{Taking false branch}} @@ -431,4 +559,3 @@ void test_escape_to_unknown_block(Blk blk) { blk(getObject()); // no-crash } -