[alpha.webkit.UncountedCallArgsChecker] Fix a false negative when a call argument is a local variable. #129974
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…all argument is a local variable. isASafeCallArg erroneously returns true when a call argument is a local variable regardless of its type. This is incorrect. We should only allow any local variable of a safe pointer type. Fix the bug by moving the logic to check for a function parameter and local variable from isASafeCallArg to a lambda in isPtrOriginSafe, and check that the local variable is a safe pointer type. Also fix a bug in isPtrOfType that it was not recognizing DeducedTemplateSpecializationType.
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesisASafeCallArg erroneously returns true when a call argument is a local variable regardless of its type. This is incorrect. We should only allow any local variable of a safe pointer type. Fix the bug by moving the logic to check for a function parameter and local variable from isASafeCallArg to a lambda in isPtrOriginSafe, and check that the local variable is a safe pointer type. Also fix a bug in isPtrOfType that it was not recognizing DeducedTemplateSpecializationType. Full diff: https://github.com/llvm/llvm-project/pull/129974.diff 4 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index dc86c4fcc64b1..73b6afd3642c7 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -141,7 +141,7 @@ bool isASafeCallArg(const Expr *E) {
assert(E);
if (auto *Ref = dyn_cast<DeclRefExpr>(E)) {
if (auto *D = dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
- if (isa<ParmVarDecl>(D) || D->isLocalVarDecl())
+ if (isa<ParmVarDecl>(D))
return true;
}
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 7899b19854806..cbb207bcb9df0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -162,13 +162,16 @@ static bool isPtrOfType(const clang::QualType T, Predicate Pred) {
type = elaboratedT->desugar();
continue;
}
- auto *SpecialT = type->getAs<TemplateSpecializationType>();
- if (!SpecialT)
- return false;
- auto *Decl = SpecialT->getTemplateName().getAsTemplateDecl();
- if (!Decl)
- return false;
- return Pred(Decl->getNameAsString());
+ if (auto *SpecialT = type->getAs<TemplateSpecializationType>()) {
+ auto *Decl = SpecialT->getTemplateName().getAsTemplateDecl();
+ if (!Decl)
+ return false;
+ return Pred(Decl->getNameAsString());
+ } else if (auto *DTS = type->getAs<DeducedTemplateSpecializationType>()) {
+ auto *Decl = DTS->getTemplateName().getAsTemplateDecl();
+ return Pred(Decl->getNameAsString());
+ } else
+ break;
}
return false;
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index d633fbcbd798b..fbb6a06afbdac 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -158,6 +158,14 @@ class RawPtrRefCallArgsChecker
// foo(NULL)
return true;
}
+ if (auto *Ref = dyn_cast<DeclRefExpr>(ArgOrigin)) {
+ if (auto *D = dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
+ if (isa<ParmVarDecl>(D))
+ return true; // Parameters are transitively safe.
+ if (D->isLocalVarDecl() && isSafePtrType(D->getType()))
+ return true;
+ }
+ }
if (isASafeCallArg(ArgOrigin))
return true;
if (EFA.isACallToEnsureFn(ArgOrigin))
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index b4613d5090f29..4fc5a574f9e69 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -374,7 +374,14 @@ namespace call_with_explicit_temporary_obj {
}
}
-namespace call_with_explicit_construct {
+namespace call_via_local_var {
+ RefCountable* provide();
+ void bar(RefCountable*);
+ void foo() {
+ auto* obj = provide();
+ bar(obj);
+ // expected-warning@-1{{Call argument is uncounted and unsafe}}
+ }
}
namespace call_with_adopt_ref {
|
Member
|
@llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesisASafeCallArg erroneously returns true when a call argument is a local variable regardless of its type. This is incorrect. We should only allow any local variable of a safe pointer type. Fix the bug by moving the logic to check for a function parameter and local variable from isASafeCallArg to a lambda in isPtrOriginSafe, and check that the local variable is a safe pointer type. Also fix a bug in isPtrOfType that it was not recognizing DeducedTemplateSpecializationType. Full diff: https://github.com/llvm/llvm-project/pull/129974.diff 4 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index dc86c4fcc64b1..73b6afd3642c7 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -141,7 +141,7 @@ bool isASafeCallArg(const Expr *E) {
assert(E);
if (auto *Ref = dyn_cast<DeclRefExpr>(E)) {
if (auto *D = dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
- if (isa<ParmVarDecl>(D) || D->isLocalVarDecl())
+ if (isa<ParmVarDecl>(D))
return true;
}
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 7899b19854806..cbb207bcb9df0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -162,13 +162,16 @@ static bool isPtrOfType(const clang::QualType T, Predicate Pred) {
type = elaboratedT->desugar();
continue;
}
- auto *SpecialT = type->getAs<TemplateSpecializationType>();
- if (!SpecialT)
- return false;
- auto *Decl = SpecialT->getTemplateName().getAsTemplateDecl();
- if (!Decl)
- return false;
- return Pred(Decl->getNameAsString());
+ if (auto *SpecialT = type->getAs<TemplateSpecializationType>()) {
+ auto *Decl = SpecialT->getTemplateName().getAsTemplateDecl();
+ if (!Decl)
+ return false;
+ return Pred(Decl->getNameAsString());
+ } else if (auto *DTS = type->getAs<DeducedTemplateSpecializationType>()) {
+ auto *Decl = DTS->getTemplateName().getAsTemplateDecl();
+ return Pred(Decl->getNameAsString());
+ } else
+ break;
}
return false;
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index d633fbcbd798b..fbb6a06afbdac 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -158,6 +158,14 @@ class RawPtrRefCallArgsChecker
// foo(NULL)
return true;
}
+ if (auto *Ref = dyn_cast<DeclRefExpr>(ArgOrigin)) {
+ if (auto *D = dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
+ if (isa<ParmVarDecl>(D))
+ return true; // Parameters are transitively safe.
+ if (D->isLocalVarDecl() && isSafePtrType(D->getType()))
+ return true;
+ }
+ }
if (isASafeCallArg(ArgOrigin))
return true;
if (EFA.isACallToEnsureFn(ArgOrigin))
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index b4613d5090f29..4fc5a574f9e69 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -374,7 +374,14 @@ namespace call_with_explicit_temporary_obj {
}
}
-namespace call_with_explicit_construct {
+namespace call_via_local_var {
+ RefCountable* provide();
+ void bar(RefCountable*);
+ void foo() {
+ auto* obj = provide();
+ bar(obj);
+ // expected-warning@-1{{Call argument is uncounted and unsafe}}
+ }
}
namespace call_with_adopt_ref {
|
Contributor
Author
|
On hindsight, the existing code is correct. We're lying on local variable checker to check the liveness of a local variable. Call arguments checker was just relying on that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
isASafeCallArg erroneously returns true when a call argument is a local variable regardless of its type. This is incorrect. We should only allow any local variable of a safe pointer type.
Fix the bug by moving the logic to check for a function parameter and local variable from isASafeCallArg to a lambda in isPtrOriginSafe, and check that the local variable is a safe pointer type.
Also fix a bug in isPtrOfType that it was not recognizing DeducedTemplateSpecializationType.