[-Wunsafe-buffer-usage] Fix false negatives of missing 2-param span ctors in constructor initializers #113226
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
…tor in constructor initializers The analysis now searches for every descendant stmt of constructor initializers and recurses if the descendant is another constructor call. (rdar://137999201)
ziqingluo-90
requested review from
danakj,
malavikasamak,
t-rasmud and
jkorous-apple
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-analysis Author: Ziqing Luo (ziqingluo-90) ChangesThe analysis now searches for every descendant stmt of constructor initializers and recurses if the descendant is another constructor call. (rdar://137999201) Full diff: https://github.com/llvm/llvm-project/pull/113226.diff 5 Files Affected:
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 267cde64f8f23c..07f1a85efbea50 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -122,7 +122,12 @@ class UnsafeBufferUsageHandler {
const Expr *UnsafeArg = nullptr) = 0;
/// Invoked when an unsafe operation with a std container is found.
- virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+ /// \param Invocation the `Stmt` that either is a direct call to the unsafe
+ /// span constructor or involves a direct call to it
+ /// \param UnsafeSpanCall the `Stmt` that refers to the direct call to the
+ /// unsafe span constructor
+ virtual void handleUnsafeOperationInContainer(const Stmt *Invocation,
+ const Stmt *UnsafeSpanCall,
bool IsRelatedToDecl,
ASTContext &Ctx) = 0;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 883db838ca0147..238446a40ce4bc 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12521,6 +12521,8 @@ def note_safe_buffer_usage_suggestions_disabled : Note<
def warn_unsafe_buffer_usage_in_container : Warning<
"the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information">,
InGroup<UnsafeBufferUsageInContainer>, DefaultIgnore;
+def note_unsafe_buffer_usage_in_container : Note<
+ "the 'std::span' constructor call here">;
#ifndef NDEBUG
// Not a user-facing diagnostic. Useful for debugging false negatives in
// -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits).
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 5e0ec9ecc92ea4..28469ab96812a3 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -455,6 +455,28 @@ AST_MATCHER_P(CallExpr, hasNumArgs, unsigned, Num) {
return Node.getNumArgs() == Num;
}
+// Matches a CXXConstructor call if it matches the `InnerMatcher` in a recursive
+// manner. I.e., either
+// 1) `Node` matches `InnerMatcher` directly, or
+// 2) a descendant of one of `Node`'s initializers matches
+// `ctorOrForEachInitializerDescendant` with the `InnerMatcher`.
+AST_MATCHER_P(CXXConstructExpr, ctorOrForEachInitializerDescendant,
+ internal::Matcher<CXXConstructExpr>, InnerMatcher) {
+ if (InnerMatcher.matches(Node, Finder, Builder))
+ return true;
+
+ const CXXConstructorDecl *CD = Node.getConstructor();
+
+ // recursive on both base and member initializers:
+ for (const CXXCtorInitializer *Init : CD->inits()) {
+ if (forEachDescendantStmt(
+ cxxConstructExpr(ctorOrForEachInitializerDescendant(InnerMatcher)))
+ .matches(*Init->getInit(), Finder, Builder))
+ return true;
+ }
+ return false;
+}
+
namespace libc_func_matchers {
// Under `libc_func_matchers`, define a set of matchers that match unsafe
// functions in libc and unsafe calls to them.
@@ -1218,38 +1240,49 @@ class PointerArithmeticGadget : public WarningGadget {
};
class SpanTwoParamConstructorGadget : public WarningGadget {
- static constexpr const char *const SpanTwoParamConstructorTag =
- "spanTwoParamConstructor";
- const CXXConstructExpr *Ctor; // the span constructor expression
+ static constexpr const char *const InvocationExprTag =
+ "spanTwoParamConstructor_Invocation";
+ static constexpr const char *const SpanTwoParamCtorTag =
+ "spanTwoParamConstructor_Ctor";
+ // The constructor call that either 1) is a direct call to the unsafe span
+ // constructor, or 2) involves a call to the unsafe span constructor
+ // (recursively through constructor initializers):
+ const CXXConstructExpr *Invocation;
+ // In case 1) above, `Ctor == Invocation`; otherwise `Ctor` refers to the
+ // actual span constructor call involved in the `Invocation`.
+ const CXXConstructExpr *Ctor;
public:
SpanTwoParamConstructorGadget(const MatchFinder::MatchResult &Result)
: WarningGadget(Kind::SpanTwoParamConstructor),
- Ctor(Result.Nodes.getNodeAs<CXXConstructExpr>(
- SpanTwoParamConstructorTag)) {}
+ Invocation(Result.Nodes.getNodeAs<CXXConstructExpr>(InvocationExprTag)),
+ Ctor(Result.Nodes.getNodeAs<CXXConstructExpr>(SpanTwoParamCtorTag)) {}
static bool classof(const Gadget *G) {
return G->getKind() == Kind::SpanTwoParamConstructor;
}
static Matcher matcher() {
- auto HasTwoParamSpanCtorDecl = hasDeclaration(
- cxxConstructorDecl(hasDeclContext(isInStdNamespace()), hasName("span"),
- parameterCountIs(2)));
-
- return stmt(cxxConstructExpr(HasTwoParamSpanCtorDecl,
- unless(isSafeSpanTwoParamConstruct()))
- .bind(SpanTwoParamConstructorTag));
+ auto HasTwoParamSpanCtorDecl =
+ cxxConstructExpr(hasDeclaration(cxxConstructorDecl(
+ hasDeclContext(isInStdNamespace()),
+ hasName("span"), parameterCountIs(2))),
+ unless(isSafeSpanTwoParamConstruct()))
+ .bind(SpanTwoParamCtorTag);
+ return stmt(cxxConstructExpr(
+ ctorOrForEachInitializerDescendant(HasTwoParamSpanCtorDecl))
+ .bind(InvocationExprTag));
}
static Matcher matcher(const UnsafeBufferUsageHandler *Handler) {
- return stmt(unless(ignoreUnsafeBufferInContainer(Handler)), matcher());
+ return stmt(unless(ignoreUnsafeBufferInContainer(Handler)),
+ SpanTwoParamConstructorGadget::matcher());
}
void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
bool IsRelatedToDecl,
ASTContext &Ctx) const override {
- Handler.handleUnsafeOperationInContainer(Ctor, IsRelatedToDecl, Ctx);
+ Handler.handleUnsafeOperationInContainer(Invocation, Ctor, IsRelatedToDecl, Ctx);
}
SourceLocation getSourceLoc() const override { return Ctor->getBeginLoc(); }
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index c76733e9a774b6..06d078d5f0550c 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2327,19 +2327,21 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
}
}
- void handleUnsafeOperationInContainer(const Stmt *Operation,
+ void handleUnsafeOperationInContainer(const Stmt *Invocation,
+ const Stmt *UnsafeSpanCall,
bool IsRelatedToDecl,
ASTContext &Ctx) override {
SourceLocation Loc;
SourceRange Range;
unsigned MsgParam = 0;
- // This function only handles SpanTwoParamConstructorGadget so far, which
- // always gives a CXXConstructExpr.
- const auto *CtorExpr = cast<CXXConstructExpr>(Operation);
- Loc = CtorExpr->getLocation();
-
- S.Diag(Loc, diag::warn_unsafe_buffer_usage_in_container);
+ Loc = Invocation->getBeginLoc();
+ S.Diag(Loc, diag::warn_unsafe_buffer_usage_in_container)
+ << Invocation->getSourceRange();
+ if (Invocation != UnsafeSpanCall)
+ S.Diag(UnsafeSpanCall->getBeginLoc(),
+ diag::note_unsafe_buffer_usage_in_container)
+ << UnsafeSpanCall->getSourceRange();
if (IsRelatedToDecl) {
assert(!SuggestSuggestions &&
"Variables blamed for unsafe buffer usage without suggestions!");
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
index e97511593bbd81..7258d4c4ef640e 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
@@ -157,3 +157,61 @@ namespace test_flag {
}
} //namespace test_flag
+
+
+namespace ctor_initializer_list {
+ class Base {
+ std::span<int> S;
+
+ public:
+ Base(int *p, unsigned n) : S(p, n) {} // expected-note 4{{the 'std::span' constructor call here}}
+ };
+
+ class Derived : public Base {
+ public:
+ Derived(int *p, unsigned n) : Base(p, n) {}
+ };
+
+ class Friend {
+ Derived D;
+ public:
+ Friend(int *p, unsigned n) : D(p, n) {}
+ };
+
+ class FriendToo {
+ Friend F;
+ public:
+ FriendToo(int *p) : F(p, 10) {}
+ };
+
+ class SpanData {
+ int * P;
+ public:
+ SpanData(int *p) : P(std::span<int>(p, 10).data()) {} // expected-note 2{{the 'std::span' constructor call here}}
+ };
+
+ class SpanDataDerived : public SpanData {
+ public:
+ SpanDataDerived(int *p) : SpanData(p) {}
+ };
+
+ void foo(int *p) {
+ Base B(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+ Derived D(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+ Friend F(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+ FriendToo FT(p); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+ SpanData SD(p); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+ SpanDataDerived SDD(p); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunsafe-buffer-usage-in-container"
+ {
+ Base B(p, 10);
+ Derived D(p, 10);
+ Friend F(p, 10);
+ FriendToo FT(p);
+ SpanData SD(p);
+ SpanDataDerived SDD(p);
+ }
+#pragma clang diagnostic pop
+ }
+} // ctor_initializer_list
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Worth to mention that there could be "false positives" introduced to those std::span calls in C'tor initializers because the safe-pattern matching is syntactic only, which doesn't know there can be "call stacks" involved in those recursive cases. |
|
This overlaps with #91991 which should probably be landed in its entirety. (It looks like it's about attributes but in fact it isn't. It's about finding all gadgets in all those new places.) I think that patch was almost ready and it was a matter of considering my fix in #91991 (comment) |
yeah, you are right. Let's hang out in that PR then. |
#91991 merged. Closing this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The analysis now searches for every descendant stmt of constructor initializers and recurses if the descendant is another constructor call.
(rdar://137999201)