[-Wunsafe-buffer-usage] Fix false negatives of missing 2-param span ctors in constructor initializers

[ziqingluo-90]

The analysis now searches for every descendant stmt of constructor initializers and recurses if the descendant is another constructor call.

(rdar://137999201)

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Ziqing Luo (ziqingluo-90)

Changes

The 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:

• (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h (+6-1)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2)
• (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+47-14)
• (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+9-7)
• (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp (+58)

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 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ziqingluo-90]

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.

[haoNoQ]

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)

[ziqingluo-90]

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.

[ziqingluo-90]

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.