diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index 8a2d56668e32f..b28f2c6b99c50 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -66,7 +66,7 @@ class UnsafeBufferUsageHandler { /// Invoked when an unsafe operation over raw pointers is found. virtual void handleUnsafeOperation(const Stmt *Operation, - bool IsRelatedToDecl) = 0; + bool IsRelatedToDecl, ASTContext &Ctx) = 0; /// Invoked when a fix is suggested against a variable. This function groups /// all variables that must be fixed together (i.e their types must be changed diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def index 757ee452ced74..c976616883651 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -30,6 +30,7 @@ WARNING_GADGET(Decrement) WARNING_GADGET(ArraySubscript) WARNING_GADGET(PointerArithmetic) WARNING_GADGET(UnsafeBufferUsageAttr) +WARNING_GADGET(DataInvocation) FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalue Context FIXABLE_GADGET(DerefSimplePtrArithFixable) FIXABLE_GADGET(PointerDereference) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index aebb7d9b945c3..e54f969c19039 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -12064,7 +12064,7 @@ def warn_unsafe_buffer_variable : Warning< InGroup, DefaultIgnore; def warn_unsafe_buffer_operation : Warning< "%select{unsafe pointer operation|unsafe pointer arithmetic|" - "unsafe buffer access|function introduces unsafe buffer manipulation}0">, + "unsafe buffer access|function introduces unsafe buffer manipulation|unsafe invocation of span::data}0">, InGroup, DefaultIgnore; def note_unsafe_buffer_operation : Note< "used%select{| in pointer arithmetic| in buffer access}0 here">; diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 70eec1cee57f8..724c4304a0724 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -721,6 +721,34 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget { DeclUseList getClaimedVarUseSites() const override { return {}; } }; +// Warning gadget for unsafe invocation of span::data method. +// Triggers when the pointer returned by the invocation is immediately +// cast to a larger type. + +class DataInvocationGadget : public WarningGadget { + constexpr static const char *const OpTag = "data_invocation_expr"; + const ExplicitCastExpr *Op; + +public: + DataInvocationGadget(const MatchFinder::MatchResult &Result) + : WarningGadget(Kind::DataInvocation), + Op(Result.Nodes.getNodeAs(OpTag)) {} + + static bool classof(const Gadget *G) { + return G->getKind() == Kind::DataInvocation; + } + + static Matcher matcher() { + return stmt( + explicitCastExpr(has(cxxMemberCallExpr(callee(cxxMethodDecl( + hasName("data"), ofClass(hasName("std::span"))))))) + .bind(OpTag)); + } + const Stmt *getBaseStmt() const override { return Op; } + + DeclUseList getClaimedVarUseSites() const override { return {}; } +}; + // Represents expressions of the form `DRE[*]` in the Unspecified Lvalue // Context (see `isInUnspecifiedLvalueContext`). // Note here `[]` is the built-in subscript operator. @@ -2657,8 +2685,8 @@ void clang::checkUnsafeBufferUsage(const Decl *D, // every problematic operation and consider it done. No need to deal // with fixable gadgets, no need to group operations by variable. for (const auto &G : WarningGadgets) { - Handler.handleUnsafeOperation(G->getBaseStmt(), - /*IsRelatedToDecl=*/false); + Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false, + D->getASTContext()); } // This return guarantees that most of the machine doesn't run when @@ -2893,7 +2921,8 @@ void clang::checkUnsafeBufferUsage(const Decl *D, Tracker, Handler, VarGrpMgr); for (const auto &G : UnsafeOps.noVar) { - Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false); + Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false, + D->getASTContext()); } for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) { @@ -2904,7 +2933,8 @@ void clang::checkUnsafeBufferUsage(const Decl *D, : FixItList{}, D); for (const auto &G : WarningGadgets) { - Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true); + Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true, + D->getASTContext()); } } } diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 0947e8b0f526a..9eb1df5f02405 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2226,8 +2226,8 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { UnsafeBufferUsageReporter(Sema &S, bool SuggestSuggestions) : S(S), SuggestSuggestions(SuggestSuggestions) {} - void handleUnsafeOperation(const Stmt *Operation, - bool IsRelatedToDecl) override { + void handleUnsafeOperation(const Stmt *Operation, bool IsRelatedToDecl, + ASTContext &Ctx) override { SourceLocation Loc; SourceRange Range; unsigned MsgParam = 0; @@ -2261,6 +2261,18 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { // note_unsafe_buffer_operation doesn't have this mode yet. assert(!IsRelatedToDecl && "Not implemented yet!"); MsgParam = 3; + } else if (const auto *ECE = dyn_cast(Operation)) { + QualType destType = ECE->getType(); + const uint64_t dSize = + Ctx.getTypeSize(destType.getTypePtr()->getPointeeType()); + if (const auto *CE = dyn_cast(ECE->getSubExpr())) { + QualType srcType = CE->getType(); + const uint64_t sSize = + Ctx.getTypeSize(srcType.getTypePtr()->getPointeeType()); + if (sSize >= dSize) + return; + } + MsgParam = 4; } Loc = Operation->getBeginLoc(); Range = Operation->getSourceRange(); diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-data-invocation.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-data-invocation.cpp new file mode 100644 index 0000000000000..79eb3bb4bacc6 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-data-invocation.cpp @@ -0,0 +1,131 @@ +// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -fblocks -include %s -verify %s + +// RUN: %clang -x c++ -frtti -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s +// RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s +// RUN: %clang_cc1 -std=c++20 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s +// CHECK-NOT: [-Wunsafe-buffer-usage] + +#ifndef INCLUDED +#define INCLUDED +#pragma clang system_header + +// no spanification warnings for system headers +#else + +namespace std { + class type_info; + class bad_cast; + class bad_typeid; +} +using size_t = __typeof(sizeof(int)); +void *malloc(size_t); + +void foo(int v) { +} + +void foo(int *p){} + +namespace std{ + template class span { + + T *elements; + + span(T *, unsigned){} + + public: + + constexpr span subspan(size_t offset, size_t count) const { + return span (elements+offset, count); // expected-warning{{unsafe pointer arithmetic}} + } + + constexpr T* data() const noexcept { + return elements; + } + + + constexpr T* hello() const noexcept { + return elements; + } +}; + + template class span_duplicate { + span_duplicate(T *, unsigned){} + + T array[10]; + + public: + + T* data() { + return array; + } + +}; +} + +using namespace std; + +class A { + int a, b, c; +}; + +class B { + int a, b, c; +}; + +struct Base { + virtual ~Base() = default; +}; + +struct Derived: Base { + int d; +}; + +void cast_without_data(int *ptr) { + A *a = (A*) ptr; + float *p = (float*) ptr; +} + +void warned_patterns(std::span span_ptr, std::span base_span, span span_without_qual) { + A *a1 = (A*)span_ptr.data(); // expected-warning{{unsafe invocation of span::data}} + a1 = (A*)span_ptr.data(); // expected-warning{{unsafe invocation of span::data}} + + A *a2 = (A*) span_without_qual.data(); // expected-warning{{unsafe invocation of span::data}} + + // TODO:: Should we warn when we cast from base to derived type? + Derived *b = dynamic_cast (base_span.data());// expected-warning{{unsafe invocation of span::data}} + + // TODO:: This pattern is safe. We can add special handling for it, if we decide this + // is the recommended fixit for the unsafe invocations. + A *a3 = (A*)span_ptr.subspan(0, sizeof(A)).data(); // expected-warning{{unsafe invocation of span::data}} +} + +void not_warned_patterns(std::span span_ptr, std::span base_span) { + int *p = (int*) span_ptr.data(); // Cast to a smaller type + + B *b = (B*) span_ptr.data(); // Cast to a type of same size. + + p = (int*) span_ptr.data(); + A *a = (A*) span_ptr.hello(); // Invoking other methods. +} + +// We do not want to warn about other types +void other_classes(std::span_duplicate span_ptr) { + int *p; + A *a = (A*)span_ptr.data(); + a = (A*)span_ptr.data(); +} + +// Potential source for false negatives + +A false_negatives(std::span span_pt, span span_A) { + int *ptr = span_pt.data(); + + A *a1 = (A*)ptr; //TODO: We want to warn here eventually. + + A *a2= span_A.data(); + return *a2; // TODO: Can cause OOB if span_pt is empty + +} +#endif