Skip to content

Commit

Permalink
[analyzer] Remove rdar links from static analyzer and libAnalysis sou…
Browse files Browse the repository at this point in the history 
…rces. 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.
  • Loading branch information
@haoNoQ
haoNoQ committed Jul 28, 2023
1 parent 38b648b commit 7f25a88
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 58 deletions.
3 changes: 1 addition & 2 deletions clang/lib/Analysis/CFG.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
48 changes: 20 additions & 28 deletions clang/lib/Analysis/RetainSummaryManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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),
Expand All @@ -324,52 +326,47 @@ 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.
ScratchArgs = AF.add(ScratchArgs, 8, ArgEffect(ArgEffect(StopTracking)));
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,
Expand All @@ -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,
Expand Down Expand Up @@ -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 &&
Expand Down Expand Up @@ -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));
Expand All @@ -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
Expand Down
34 changes: 22 additions & 12 deletions clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//===----------------------------------------------------------------------===//

Expand Down Expand Up @@ -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
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -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
//===----------------------------------------------------------------------===//

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 0 additions & 3 deletions clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down
5 changes: 3 additions & 2 deletions clang/lib/StaticAnalyzer/Core/CallEvent.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {};
}
Expand Down
35 changes: 24 additions & 11 deletions clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down

0 comments on commit 7f25a88

Please sign in to comment.