Skip to content

Commit 1b169f4

Browse files
committed
[-Wunsafe-buffer-usage] Add a new warning for use of two-parameter std::span constructors
Constructing `std::span` objects with the two parameter constructors could introduce mismatched bounds information, which defeats the purpose of using `std::span`. Therefore, we warn every use of such constructors. We also plan to incrementally teach `-Wunsafe-buffer-usage` about benign usages of those constructors. rdar://115817781
1 parent 4f215fd commit 1b169f4

File tree

7 files changed

+214
-4
lines changed

7 files changed

+214
-4
lines changed

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
#include "clang/AST/Decl.h"
1818
#include "clang/AST/Stmt.h"
19+
#include "clang/Basic/SourceLocation.h"
1920
#include "llvm/Support/Debug.h"
2021

2122
namespace clang {
@@ -98,9 +99,14 @@ class UnsafeBufferUsageHandler {
9899
#endif
99100

100101
public:
101-
/// Returns a reference to the `Preprocessor`:
102+
/// \return true iff buffer safety is opt-out at `Loc`; false otherwise.
102103
virtual bool isSafeBufferOptOut(const SourceLocation &Loc) const = 0;
103104

105+
/// \return true iff unsafe uses in containers should NOT be reported at
106+
/// `Loc`; false otherwise.
107+
virtual bool
108+
ignoreUnsafeBufferInContainer(const SourceLocation &Loc) const = 0;
109+
104110
virtual std::string
105111
getUnsafeBufferUsageAttributeTextAt(SourceLocation Loc,
106112
StringRef WSSuffix = "") const = 0;

clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,12 @@
1818
#define WARNING_GADGET(name) GADGET(name)
1919
#endif
2020

21+
/// A `WARNING_GADGET` subset, where the code pattern of each gadget
22+
/// corresponds uses of a (possibly hardened) contatiner (e.g., `std::span`).
23+
#ifndef WARNING_CONTAINER_GADGET
24+
#define WARNING_CONTAINER_GADGET(name) WARNING_GADGET(name)
25+
#endif
26+
2127
/// Safe gadgets correspond to code patterns that aren't unsafe but need to be
2228
/// properly recognized in order to emit correct warnings and fixes over unsafe
2329
/// gadgets.
@@ -31,6 +37,7 @@ WARNING_GADGET(ArraySubscript)
3137
WARNING_GADGET(PointerArithmetic)
3238
WARNING_GADGET(UnsafeBufferUsageAttr)
3339
WARNING_GADGET(DataInvocation)
40+
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0, arg1)`
3441
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalue Context
3542
FIXABLE_GADGET(DerefSimplePtrArithFixable)
3643
FIXABLE_GADGET(PointerDereference)
@@ -43,4 +50,5 @@ FIXABLE_GADGET(PointerInit)
4350

4451
#undef FIXABLE_GADGET
4552
#undef WARNING_GADGET
53+
#undef WARNING_CONTAINER_GADGET
4654
#undef GADGET

clang/include/clang/Basic/DiagnosticSemaKinds.td

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12075,6 +12075,9 @@ def note_unsafe_buffer_variable_fixit_together : Note<
1207512075
"%select{|, and change %2 to safe types to make function %4 bounds-safe}3">;
1207612076
def note_safe_buffer_usage_suggestions_disabled : Note<
1207712077
"pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">;
12078+
def warn_unsafe_buffer_usage_in_container : Warning<
12079+
"the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information">,
12080+
InGroup<UnsafeBufferUsageInContainer>, DefaultIgnore;
1207812081
#ifndef NDEBUG
1207912082
// Not a user-facing diagnostic. Useful for debugging false negatives in
1208012083
// -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits).

clang/lib/Analysis/UnsafeBufferUsage.cpp

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,11 @@ AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const UnsafeBufferUsageHandler *,
232232
return !Handler->isSafeBufferOptOut(Node.getBeginLoc());
233233
}
234234

235+
AST_MATCHER_P(Stmt, ignoreUnsafeBufferInContainer,
236+
const UnsafeBufferUsageHandler *, Handler) {
237+
return Handler->ignoreUnsafeBufferInContainer(Node.getBeginLoc());
238+
}
239+
235240
AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
236241
return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
237242
}
@@ -594,6 +599,43 @@ class PointerArithmeticGadget : public WarningGadget {
594599
// FIXME: this gadge will need a fix-it
595600
};
596601

602+
class SpanTwoParamConstructorGadget : public WarningGadget {
603+
static constexpr const char *const SpanTwoParamConstructorTag =
604+
"spanTwoParamConstructor";
605+
const CXXConstructExpr *Ctor; // the span constructor expression
606+
607+
public:
608+
SpanTwoParamConstructorGadget(const MatchFinder::MatchResult &Result)
609+
: WarningGadget(Kind::SpanTwoParamConstructor),
610+
Ctor(Result.Nodes.getNodeAs<CXXConstructExpr>(
611+
SpanTwoParamConstructorTag)) {}
612+
613+
static bool classof(const Gadget *G) {
614+
return G->getKind() == Kind::SpanTwoParamConstructor;
615+
}
616+
617+
static Matcher matcher() {
618+
auto HasTwoParamSpanCtorDecl = hasDeclaration(
619+
cxxConstructorDecl(hasDeclContext(isInStdNamespace()), hasName("span"),
620+
parameterCountIs(2)));
621+
622+
return stmt(cxxConstructExpr(HasTwoParamSpanCtorDecl)
623+
.bind(SpanTwoParamConstructorTag));
624+
}
625+
626+
const Stmt *getBaseStmt() const override { return Ctor; }
627+
628+
DeclUseList getClaimedVarUseSites() const override {
629+
// If the constructor call is of the form `std::span{var, n}`, `var` is
630+
// considered an unsafe variable.
631+
if (auto *DRE = dyn_cast<DeclRefExpr>(Ctor->getArg(0))) {
632+
if (isa<VarDecl>(DRE->getDecl()))
633+
return {DRE};
634+
}
635+
return {};
636+
}
637+
};
638+
597639
/// A pointer initialization expression of the form:
598640
/// \code
599641
/// int *p = q;
@@ -1209,6 +1251,10 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
12091251
#define WARNING_GADGET(x) \
12101252
allOf(x ## Gadget::matcher().bind(#x), \
12111253
notInSafeBufferOptOut(&Handler)),
1254+
#define WARNING_CONTAINER_GADGET(x) \
1255+
allOf(x ## Gadget::matcher().bind(#x), \
1256+
notInSafeBufferOptOut(&Handler), \
1257+
unless(ignoreUnsafeBufferInContainer(&Handler))),
12121258
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
12131259
// Avoid a hanging comma.
12141260
unless(stmt())

clang/lib/Sema/AnalysisBasedWarnings.cpp

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
#include "clang/AST/StmtCXX.h"
2727
#include "clang/AST/StmtObjC.h"
2828
#include "clang/AST/StmtVisitor.h"
29-
#include "clang/AST/RecursiveASTVisitor.h"
3029
#include "clang/AST/Type.h"
3130
#include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h"
3231
#include "clang/Analysis/Analyses/CalledOnceCheck.h"
@@ -39,6 +38,7 @@
3938
#include "clang/Analysis/CFG.h"
4039
#include "clang/Analysis/CFGStmtMap.h"
4140
#include "clang/Basic/Diagnostic.h"
41+
#include "clang/Basic/DiagnosticSema.h"
4242
#include "clang/Basic/SourceLocation.h"
4343
#include "clang/Basic/SourceManager.h"
4444
#include "clang/Lex/Preprocessor.h"
@@ -2256,6 +2256,9 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
22562256
Range = UO->getSubExpr()->getSourceRange();
22572257
MsgParam = 1;
22582258
}
2259+
} else if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(Operation)) {
2260+
S.Diag(CtorExpr->getLocation(),
2261+
diag::warn_unsafe_buffer_usage_in_container);
22592262
} else {
22602263
if (isa<CallExpr>(Operation)) {
22612264
// note_unsafe_buffer_operation doesn't have this mode yet.
@@ -2331,6 +2334,10 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
23312334
return S.PP.isSafeBufferOptOut(S.getSourceManager(), Loc);
23322335
}
23332336

2337+
bool ignoreUnsafeBufferInContainer(const SourceLocation &Loc) const override {
2338+
return S.Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_container, Loc);
2339+
}
2340+
23342341
// Returns the text representation of clang::unsafe_buffer_usage attribute.
23352342
// `WSSuffix` holds customized "white-space"s, e.g., newline or whilespace
23362343
// characters.
@@ -2495,6 +2502,8 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
24952502
if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation,
24962503
Node->getBeginLoc()) ||
24972504
!Diags.isIgnored(diag::warn_unsafe_buffer_variable,
2505+
Node->getBeginLoc()) ||
2506+
!Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_container,
24982507
Node->getBeginLoc())) {
24992508
clang::checkUnsafeBufferUsage(Node, R,
25002509
UnsafeBufferUsageShouldEmitSuggestions);
@@ -2505,7 +2514,9 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
25052514
// Emit per-function analysis-based warnings that require the whole-TU
25062515
// reasoning. Check if any of them is enabled at all before scanning the AST:
25072516
if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, SourceLocation()) ||
2508-
!Diags.isIgnored(diag::warn_unsafe_buffer_variable, SourceLocation())) {
2517+
!Diags.isIgnored(diag::warn_unsafe_buffer_variable, SourceLocation()) ||
2518+
!Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_container,
2519+
SourceLocation())) {
25092520
CallableVisitor(CallAnalyzers).TraverseTranslationUnitDecl(TU);
25102521
}
25112522
}
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage-in-container -verify %s
2+
3+
namespace std {
4+
template <class T> class span {
5+
public:
6+
constexpr span(T *, unsigned){}
7+
8+
template<class Begin, class End>
9+
constexpr span(Begin first, End last){}
10+
11+
T * data();
12+
13+
constexpr span() {};
14+
15+
constexpr span(const std::span<T> &span) {};
16+
17+
template<class R>
18+
constexpr span(R && range){};
19+
};
20+
21+
22+
template< class T >
23+
T&& move( T&& t ) noexcept;
24+
}
25+
26+
namespace irrelevant_constructors {
27+
void non_two_param_constructors() {
28+
class Array {
29+
} a;
30+
std::span<int> S; // no warn
31+
std::span<int> S1{}; // no warn
32+
std::span<int> S2{std::move(a)}; // no warn
33+
std::span<int> S3{S2}; // no warn
34+
}
35+
} // irrelevant_constructors
36+
37+
namespace construct_wt_ptr_size {
38+
std::span<int> warnVarInit(int *p) {
39+
std::span<int> S{p, 10}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
40+
std::span<int> S1(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
41+
std::span<int> S2 = std::span{p, 10}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
42+
std::span<int> S3 = std::span(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
43+
std::span<int> S4 = std::span<int>{p, 10}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
44+
std::span<int> S5 = std::span<int>(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
45+
std::span<int> S6 = {p, 10}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
46+
auto S7 = std::span<int>{p, 10}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
47+
auto S8 = std::span<int>(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
48+
const auto &S9 = std::span<int>{p, 10}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
49+
auto &&S10 = std::span<int>(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
50+
51+
#define Ten 10
52+
53+
std::span S11 = std::span<int>{p, Ten}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
54+
55+
if (auto X = std::span<int>{p, Ten}; S10.data()) { // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
56+
}
57+
58+
auto X = warnVarInit(p); // function return is fine
59+
return S;
60+
}
61+
62+
template<typename T>
63+
void foo(const T &, const T &&, T);
64+
65+
std::span<int> warnTemp(int *p) {
66+
foo(std::span<int>{p, 10}, // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
67+
std::move(std::span<int>{p, 10}), // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
68+
std::span<int>{p, 10}); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
69+
70+
std::span<int> Arr[1] = {std::span<int>{p, 10}}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
71+
72+
if (std::span<int>{p, 10}.data()) { // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
73+
}
74+
return std::span<int>{p, 10}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
75+
}
76+
} // namespace construct_wt_ptr_size
77+
78+
namespace construct_wt_begin_end {
79+
class It {};
80+
81+
std::span<int> warnVarInit(It &First, It &Last) {
82+
std::span<int> S{First, Last}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
83+
std::span<int> S1(First, Last); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
84+
std::span<int> S2 = std::span<int>{First, Last}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
85+
std::span<int> S3 = std::span<int>(First, Last); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
86+
std::span<int> S4 = std::span<int>{First, Last}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
87+
std::span<int> S5 = std::span<int>(First, Last); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
88+
std::span<int> S6 = {First, Last}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
89+
auto S7 = std::span<int>{First, Last}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
90+
auto S8 = std::span<int>(First, Last); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
91+
const auto &S9 = std::span<int>{First, Last}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
92+
auto &&S10 = std::span<int>(First, Last); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
93+
94+
if (auto X = std::span<int>{First, Last}; S10.data()) { // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
95+
}
96+
97+
auto X = warnVarInit(First, Last); // function return is fine
98+
return S;
99+
}
100+
101+
template<typename T>
102+
void foo(const T &, const T &&, T);
103+
104+
std::span<int> warnTemp(It &First, It &Last) {
105+
foo(std::span<int>{First, Last}, // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
106+
std::move(std::span<int>{First, Last}), // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
107+
std::span<int>{First, Last}); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
108+
109+
std::span<int> Arr[1] = {std::span<int>{First, Last}}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
110+
111+
if (std::span<int>{First, Last}.data()) { // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
112+
}
113+
return std::span<int>{First, Last}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
114+
}
115+
} // namespace construct_wt_begin_end
116+
117+
namespace test_flag {
118+
void f(int *p) {
119+
#pragma clang diagnostic push
120+
#pragma clang diagnostic ignored "-Wunsafe-buffer-usage" // this flag turns off every unsafe-buffer warning
121+
std::span<int> S{p, 10}; // no-warning
122+
p++; // no-warning
123+
#pragma clang diagnostic pop
124+
125+
#pragma clang diagnostic push
126+
#pragma clang diagnostic warning "-Wunsafe-buffer-usage"
127+
#pragma clang diagnostic ignored "-Wunsafe-buffer-usage-in-container"
128+
// turn on all unsafe-buffer warnings except for the ones under `-Wunsafe-buffer-usage-in-container`
129+
std::span<int> S2{p, 10}; // no-warning
130+
131+
p++; // expected-warning{{unsafe pointer arithmetic}}\
132+
expected-note{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
133+
#pragma clang diagnostic pop
134+
135+
}
136+
} //namespace test_flag

clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-data-invocation.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
1+
// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -Wno-unsafe-buffer-usage-in-container\
22
// RUN: -fsafe-buffer-usage-suggestions \
33
// RUN: -fblocks -include %s -verify %s
44

0 commit comments

Comments
 (0)