[clang] 7f25a88 - [analyzer] Remove rdar links from static analyzer and libAnalysis sources. NFC.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 27 17:52:06 PDT 2023
Author: Artem Dergachev
Date: 2023-07-27T17:51:49-07:00
New Revision: 7f25a882616263108d99f7696407b725351324de
URL: https://github.com/llvm/llvm-project/commit/7f25a882616263108d99f7696407b725351324de
DIFF: https://github.com/llvm/llvm-project/commit/7f25a882616263108d99f7696407b725351324de.diff
LOG: [analyzer] Remove rdar links from static analyzer and libAnalysis sources. NFC.
I actually visited each link and added relevant context directly to the code.
This is related to the effort to eliminate internal bug tracker links
(d618f1c, e0ac46e).
Test files still have a lot of rdar links and ids in them.
I haven't touched them yet.
Added:
Modified:
clang/lib/Analysis/CFG.cpp
clang/lib/Analysis/RetainSummaryManager.cpp
clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 64a4fffaea5de0..0019f971aac4dc 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 / <rdar://problem/12156507>
+ // 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 8c997b645f155c..dfa9454c93c613 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: <rdar://problem/7299394> and <rdar://problem/11282706>.
- // 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: <rdar://problem/39390714>.
+ // 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 <rdar://problem/6961230>. (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: <rdar://problem/6326900>
- // 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 <rdar://problem/6961230>. (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: <rdar://problem/7283567>
// 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: <rdar://problem/7358899>
+ // 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: <rdar://problem/7283567>
- // 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") {
- // <rdar://problem/11059275> - 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.
- // <rdar://problem/13783514> - 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: <rdar://problem/39390714>.
- // 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 <rdar://problem/6062711>.
- // 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.
- // <rdar://problem/12858915>
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 18d575041ba74c..078b0379f5e0fa 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: <rdar://problem/6336718>
// 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: <rdar://problem/6335715>
+// 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: <rdar://problem/63371000>
+// 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: <rdar://problem/63371000>
-//===----------------------------------------------------------------------===//
-
+// 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: <rdar://problem/6337132>
+// 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 379163e12787f1..c3acb73ba7175b 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<ObjCMethodDecl>(D) ? " is returned from a method "
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index 195940e5e64339..ad5bb66c4fff3c 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. <rdar://problem/12287087>
- //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 8072531ef6fded..f075df3ab5e4d6 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 (<rdar://problem/11733396>).
- // 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()) {
More information about the cfe-commits
mailing list