diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 64a4fffaea5de..0019f971aac4d 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -2224,8 +2224,7 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc, // FIXME: The expression inside a CXXDefaultArgExpr is owned by the // called function's declaration, not by the caller. If we simply add // this expression to the CFG, we could end up with the same Expr - // appearing multiple times. - // PR13385 / + // appearing multiple times (PR13385). // // It's likewise possible for multiple CXXDefaultInitExprs for the same // expression to be used in the same function (through aggregate diff --git a/clang/lib/Analysis/RetainSummaryManager.cpp b/clang/lib/Analysis/RetainSummaryManager.cpp index 8c997b645f155..dfa9454c93c61 100644 --- a/clang/lib/Analysis/RetainSummaryManager.cpp +++ b/clang/lib/Analysis/RetainSummaryManager.cpp @@ -301,8 +301,9 @@ const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject( std::string RetTyName = RetTy.getAsString(); if (FName == "pthread_create" || FName == "pthread_setspecific") { - // Part of: and . - // This will be addressed better with IPA. + // It's not uncommon to pass a tracked object into the thread + // as 'void *arg', and then release it inside the thread. + // FIXME: We could build a much more precise model for these functions. return getPersistentStopSummary(); } else if(FName == "NSMakeCollectable") { // Handle: id NSMakeCollectable(CFTypeRef) @@ -311,7 +312,8 @@ const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject( : getPersistentStopSummary(); } else if (FName == "CMBufferQueueDequeueAndRetain" || FName == "CMBufferQueueDequeueIfDataReadyAndRetain") { - // Part of: . + // These API functions are known to NOT act as a CFRetain wrapper. + // They simply make a new object owned by the caller. return getPersistentSummary(RetEffect::MakeOwned(ObjKind::CF), ScratchArgs, ArgEffect(DoNothing), @@ -324,40 +326,39 @@ const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject( FName == "IOServiceNameMatching" || FName == "IORegistryEntryIDMatching" || FName == "IOOpenFirmwarePathMatching"))) { - // Part of . (IOKit) - // This should be addressed using a API table. + // Yes, these IOKit functions return CF objects. + // They also violate the CF naming convention. return getPersistentSummary(RetEffect::MakeOwned(ObjKind::CF), ScratchArgs, ArgEffect(DoNothing), ArgEffect(DoNothing)); } else if (FName == "IOServiceGetMatchingService" || FName == "IOServiceGetMatchingServices") { - // FIXES: - // This should be addressed using a API table. This strcmp is also - // a little gross, but there is no need to super optimize here. + // These IOKit functions accept CF objects as arguments. + // They also consume them without an appropriate annotation. ScratchArgs = AF.add(ScratchArgs, 1, ArgEffect(DecRef, ObjKind::CF)); return getPersistentSummary(RetEffect::MakeNoRet(), ScratchArgs, ArgEffect(DoNothing), ArgEffect(DoNothing)); } else if (FName == "IOServiceAddNotification" || FName == "IOServiceAddMatchingNotification") { - // Part of . (IOKit) - // This should be addressed using a API table. + // More IOKit functions suddenly accepting (and even more suddenly, + // consuming) CF objects. ScratchArgs = AF.add(ScratchArgs, 2, ArgEffect(DecRef, ObjKind::CF)); return getPersistentSummary(RetEffect::MakeNoRet(), ScratchArgs, ArgEffect(DoNothing), ArgEffect(DoNothing)); } else if (FName == "CVPixelBufferCreateWithBytes") { - // FIXES: // Eventually this can be improved by recognizing that the pixel // buffer passed to CVPixelBufferCreateWithBytes is released via // a callback and doing full IPA to make sure this is done correctly. - // FIXME: This function has an out parameter that returns an + // Note that it's passed as a 'void *', so it's hard to annotate. + // FIXME: This function also has an out parameter that returns an // allocated object. ScratchArgs = AF.add(ScratchArgs, 7, ArgEffect(StopTracking)); return getPersistentSummary(RetEffect::MakeNoRet(), ScratchArgs, ArgEffect(DoNothing), ArgEffect(DoNothing)); } else if (FName == "CGBitmapContextCreateWithData") { - // FIXES: + // This is similar to the CVPixelBufferCreateWithBytes situation above. // Eventually this can be improved by recognizing that 'releaseInfo' // passed to CGBitmapContextCreateWithData is released via // a callback and doing full IPA to make sure this is done correctly. @@ -365,11 +366,7 @@ const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject( return getPersistentSummary(RetEffect::MakeOwned(ObjKind::CF), ScratchArgs, ArgEffect(DoNothing), ArgEffect(DoNothing)); } else if (FName == "CVPixelBufferCreateWithPlanarBytes") { - // FIXES: - // Eventually this can be improved by recognizing that the pixel - // buffer passed to CVPixelBufferCreateWithPlanarBytes is released - // via a callback and doing full IPA to make sure this is done - // correctly. + // Same as CVPixelBufferCreateWithBytes, just more arguments. ScratchArgs = AF.add(ScratchArgs, 12, ArgEffect(StopTracking)); return getPersistentSummary(RetEffect::MakeNoRet(), ScratchArgs, @@ -386,12 +383,10 @@ const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject( ArgEffect(DoNothing), ArgEffect(DoNothing)); } else if (FName == "dispatch_set_context" || FName == "xpc_connection_set_context") { - // - The analyzer currently doesn't have - // a good way to reason about the finalizer function for libdispatch. + // The analyzer currently doesn't have a good way to reason about + // dispatch_set_finalizer_f() which typically cleans up the context. // If we pass a context object that is memory managed, stop tracking it. - // - Same problem, but for XPC. - // FIXME: this hack should possibly go away once we can handle - // libdispatch and XPC finalizers. + // Same with xpc_connection_set_finalizer_f(). ScratchArgs = AF.add(ScratchArgs, 1, ArgEffect(StopTracking)); return getPersistentSummary(RetEffect::MakeNoRet(), ScratchArgs, @@ -740,8 +735,8 @@ RetainSummaryManager::canEval(const CallExpr *CE, const FunctionDecl *FD, // It's okay to be a little sloppy here. if (FName == "CMBufferQueueDequeueAndRetain" || FName == "CMBufferQueueDequeueIfDataReadyAndRetain") { - // Part of: . - // These are not retain. They just return something and retain it. + // These API functions are known to NOT act as a CFRetain wrapper. + // They simply make a new object owned by the caller. return std::nullopt; } if (CE->getNumArgs() == 1 && @@ -1243,8 +1238,6 @@ void RetainSummaryManager::InitializeMethodSummaries() { // FIXME: For now we opt for false negatives with NSWindow, as these objects // self-own themselves. However, they only do this once they are displayed. // Thus, we need to track an NSWindow's display status. - // This is tracked in . - // See also http://llvm.org/bugs/show_bug.cgi?id=3714. const RetainSummary *NoTrackYet = getPersistentSummary(RetEffect::MakeNoRet(), ScratchArgs, ArgEffect(StopTracking), ArgEffect(StopTracking)); @@ -1259,7 +1252,6 @@ void RetainSummaryManager::InitializeMethodSummaries() { // For NSNull, objects returned by +null are singletons that ignore // retain/release semantics. Just don't track them. - // addClassMethSummary("NSNull", "null", NoTrackYet); // Don't track allocated autorelease pools, as it is okay to prematurely diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp index 18d575041ba74..078b0379f5e0f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp @@ -219,7 +219,6 @@ void WalkAST::VisitForStmt(ForStmt *FS) { //===----------------------------------------------------------------------===// // Check: floating point variable used as loop counter. -// Originally: // Implements: CERT security coding advisory FLP-30. //===----------------------------------------------------------------------===// @@ -467,8 +466,8 @@ void WalkAST::checkCall_bzero(const CallExpr *CE, const FunctionDecl *FD) { //===----------------------------------------------------------------------===// -// Check: Any use of 'gets' is insecure. -// Originally: +// Check: Any use of 'gets' is insecure. Most man pages literally says this. +// // Implements (part of): 300-BSI (buildsecurityin.us-cert.gov) // CWE-242: Use of Inherently Dangerous Function //===----------------------------------------------------------------------===// @@ -847,8 +846,13 @@ bool WalkAST::checkCall_strCommon(const CallExpr *CE, const FunctionDecl *FD) { } //===----------------------------------------------------------------------===// -// Check: Linear congruent random number generators should not be used -// Originally: +// Check: Linear congruent random number generators should not be used, +// i.e. rand(), random(). +// +// E. Bach, "Efficient prediction of Marsaglia-Zaman random number generators," +// in IEEE Transactions on Information Theory, vol. 44, no. 3, pp. 1253-1257, +// May 1998, https://doi.org/10.1109/18.669305 +// // CWE-338: Use of cryptographically weak prng //===----------------------------------------------------------------------===// @@ -890,11 +894,7 @@ void WalkAST::checkCall_rand(const CallExpr *CE, const FunctionDecl *FD) { CE->getCallee()->getSourceRange()); } -//===----------------------------------------------------------------------===// -// Check: 'random' should not be used -// Originally: -//===----------------------------------------------------------------------===// - +// See justification for rand(). void WalkAST::checkCall_random(const CallExpr *CE, const FunctionDecl *FD) { if (!CheckRand || !filter.check_rand) return; @@ -990,8 +990,18 @@ void WalkAST::checkMsg_decodeValueOfObjCType(const ObjCMessageExpr *ME) { } //===----------------------------------------------------------------------===// -// Check: Should check whether privileges are dropped successfully. -// Originally: +// Check: The caller should always verify that the privileges +// were dropped successfully. +// +// Some library functions, like setuid() and setgid(), should always be used +// with a check of the return value to verify that the function completed +// successfully. If the drop fails, the software will continue to run +// with the raised privileges, which might provide additional access +// to unprivileged users. +// +// (Note that this check predates __attribute__((warn_unused_result)). +// Do we still need it now that we have a compiler warning for this? +// Are these standard functions already annotated this way?) //===----------------------------------------------------------------------===// void WalkAST::checkUncheckedReturnValue(CallExpr *CE) { diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp index 379163e12787f..c3acb73ba7175 100644 --- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp @@ -786,9 +786,6 @@ RefLeakReportVisitor::getEndPath(BugReporterContext &BRC, assert(RV); if (RV->getKind() == RefVal::ErrorLeakReturned) { - // FIXME: Per comments in rdar://6320065, "create" only applies to CF - // objects. Only "copy", "alloc", "retain" and "new" transfer ownership - // to the caller for NS objects. const Decl *D = &EndN->getCodeDecl(); os << (isa(D) ? " is returned from a method " diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp index 195940e5e6433..ad5bb66c4fff3 100644 --- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp +++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp @@ -765,8 +765,9 @@ RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const { // the static type. However, because we currently don't update // DynamicTypeInfo when an object is cast, we can't actually be sure the // DynamicTypeInfo is up to date. This assert should be re-enabled once - // this is fixed. - //assert(!MD->getParent()->isDerivedFrom(RD) && "Bad DynamicTypeInfo"); + // this is fixed. + // + // assert(!MD->getParent()->isDerivedFrom(RD) && "Bad DynamicTypeInfo"); return {}; } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index 8072531ef6fde..f075df3ab5e4d 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -167,19 +167,32 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME, // intentionally drops coverage in order to prevent false alarms // in the following scenario: // - // id result = [o someMethod] - // if (result) { - // if (!o) { - // // <-- This program point should be unreachable because if o is nil - // // it must the case that result is nil as well. + // id result = [o someMethod] + // if (result) { + // if (!o) { + // // <-- This program point should be unreachable because if o is nil + // // it must the case that result is nil as well. + // } // } - // } // - // We could avoid dropping coverage by performing an explicit case split - // on each method call -- but this would get very expensive. An alternative - // would be to introduce lazy constraints. - // FIXME: This ignores many potential bugs (). - // Revisit once we have lazier constraints. + // However, it also loses coverage of the nil path prematurely, + // leading to missed reports. + // + // It's possible to handle this by performing a state split on every call: + // explore the state where the receiver is non-nil, and independently + // explore the state where it's nil. But this is not only slow, but + // completely unwarranted. The mere presence of the message syntax in the code + // isn't sufficient evidence that nil is a realistic possibility. + // + // An ideal solution would be to add the following constraint that captures + // both possibilities without splitting the state: + // + // ($x == 0) => ($y == 0) (1) + // + // where in our case '$x' is the receiver symbol, '$y' is the returned symbol, + // and '=>' is logical implication. But RangeConstraintManager can't handle + // such constraints yet, so for now we go with a simpler, more restrictive + // constraint: $x != 0, from which (1) follows as a vacuous truth. if (Msg->isInstanceMessage()) { SVal recVal = Msg->getReceiverSVal(); if (!recVal.isUndef()) {