diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index 9b53f1dc1d759..ea41eb3becfcf 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -14,6 +14,7 @@ #ifndef LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H #define LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H +#include "clang/AST/ASTTypeTraits.h" #include "clang/AST/Decl.h" #include "clang/AST/Expr.h" #include "clang/AST/Stmt.h" @@ -139,6 +140,12 @@ class UnsafeBufferUsageHandler { FixItList &&Fixes, const Decl *D, const FixitStrategy &VarTargetTypes) = 0; + // Invoked when an array subscript operator[] is used on a + // std::unique_ptr. + virtual void handleUnsafeUniquePtrArrayAccess(const DynTypedNode &Node, + bool IsRelatedToDecl, + ASTContext &Ctx) = 0; + #ifndef NDEBUG public: bool areDebugNotesRequested() { diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def index 09fa510bc0472..fae5f8b8aa8e3 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -38,6 +38,7 @@ WARNING_GADGET(PointerArithmetic) WARNING_GADGET(UnsafeBufferUsageAttr) WARNING_GADGET(UnsafeBufferUsageCtorAttr) WARNING_GADGET(DataInvocation) +WARNING_GADGET(UniquePtrArrayAccess) WARNING_OPTIONAL_GADGET(UnsafeLibcFunctionCall) WARNING_OPTIONAL_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0, arg1)` FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalue Context diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 0c994e0b5ca4d..4b27a42c6dd81 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -1750,7 +1750,8 @@ def ReadOnlyPlacementChecks : DiagGroup<"read-only-types">; // Warnings and fixes to support the "safe buffers" programming model. def UnsafeBufferUsageInContainer : DiagGroup<"unsafe-buffer-usage-in-container">; def UnsafeBufferUsageInLibcCall : DiagGroup<"unsafe-buffer-usage-in-libc-call">; -def UnsafeBufferUsage : DiagGroup<"unsafe-buffer-usage", [UnsafeBufferUsageInContainer, UnsafeBufferUsageInLibcCall]>; +def UnsafeBufferUsageInUniquePtrArrayAccess : DiagGroup<"unsafe-buffer-usage-in-unique-ptr-array-access">; +def UnsafeBufferUsage : DiagGroup<"unsafe-buffer-usage", [UnsafeBufferUsageInContainer, UnsafeBufferUsageInLibcCall, UnsafeBufferUsageInUniquePtrArrayAccess]>; // Warnings and notes InstallAPI verification. def InstallAPIViolation : DiagGroup<"installapi-violation">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index b157cbb0b8069..5be63c027cba7 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -13295,6 +13295,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, DefaultIgnore; +def warn_unsafe_buffer_usage_unique_ptr_array_access : Warning<"direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking">, + InGroup, DefaultIgnore; #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 ad3d2346d18be..f5a368636c43d 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -13,6 +13,7 @@ #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" +#include "clang/AST/DeclTemplate.h" #include "clang/AST/DynamicRecursiveASTVisitor.h" #include "clang/AST/Expr.h" #include "clang/AST/FormatString.h" @@ -1318,6 +1319,97 @@ static bool isSupportedVariable(const DeclRefExpr &Node) { return D != nullptr && isa(D); } +// Returns true for RecordDecl of type std::unique_ptr +static bool isUniquePtrArray(const CXXRecordDecl *RecordDecl) { + if (!RecordDecl || !RecordDecl->isInStdNamespace() || + RecordDecl->getNameAsString() != "unique_ptr") + return false; + + const ClassTemplateSpecializationDecl *class_template_specialization_decl = + dyn_cast(RecordDecl); + if (!class_template_specialization_decl) + return false; + + const TemplateArgumentList &template_args = + class_template_specialization_decl->getTemplateArgs(); + if (template_args.size() == 0) + return false; + + const TemplateArgument &first_arg = template_args[0]; + if (first_arg.getKind() != TemplateArgument::Type) + return false; + + QualType referred_type = first_arg.getAsType(); + return referred_type->isArrayType(); +} + +class UniquePtrArrayAccessGadget : public WarningGadget { +private: + static constexpr const char *const AccessorTag = "unique_ptr_array_access"; + const CXXOperatorCallExpr *AccessorExpr; + +public: + UniquePtrArrayAccessGadget(const MatchResult &Result) + : WarningGadget(Kind::UniquePtrArrayAccess), + AccessorExpr(Result.getNodeAs(AccessorTag)) { + assert(AccessorExpr && + "UniquePtrArrayAccessGadget requires a matched CXXOperatorCallExpr"); + } + + static bool classof(const Gadget *G) { + return G->getKind() == Kind::UniquePtrArrayAccess; + } + + static bool matches(const Stmt *S, const ASTContext &Ctx, + MatchResult &Result) { + + const CXXOperatorCallExpr *OpCall = dyn_cast(S); + if (!OpCall || OpCall->getOperator() != OO_Subscript) + return false; + + const Expr *Callee = OpCall->getCallee()->IgnoreParenImpCasts(); + if (!Callee) + return false; + + const CXXMethodDecl *Method = + dyn_cast_or_null(OpCall->getDirectCallee()); + if (!Method) + return false; + + if (Method->getOverloadedOperator() != OO_Subscript) + return false; + + const CXXRecordDecl *RecordDecl = Method->getParent(); + if (!isUniquePtrArray(RecordDecl)) + return false; + + const Expr *IndexExpr = OpCall->getArg(1); + clang::Expr::EvalResult Eval; + + // Allow [0] + if (IndexExpr->EvaluateAsInt(Eval, Ctx) && Eval.Val.getInt().isZero()) + return false; + + Result.addNode(AccessorTag, DynTypedNode::create(*OpCall)); + return true; + } + void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const override { + Handler.handleUnsafeUniquePtrArrayAccess( + DynTypedNode::create(*AccessorExpr), IsRelatedToDecl, Ctx); + } + + SourceLocation getSourceLoc() const override { + if (AccessorExpr) + return AccessorExpr->getOperatorLoc(); + return SourceLocation(); + } + + DeclUseList getClaimedVarUseSites() const override { return {}; } + SmallVector getUnsafePtrs() const override { return {}; } +}; + using FixableGadgetList = std::vector>; using WarningGadgetList = std::vector>; @@ -2632,10 +2724,13 @@ std::set clang::findUnsafePointers(const FunctionDecl *FD) { const VariableGroupsManager &, FixItList &&, const Decl *, const FixitStrategy &) override {} - bool isSafeBufferOptOut(const SourceLocation &) const override { + void handleUnsafeUniquePtrArrayAccess(const DynTypedNode &Node, + bool IsRelatedToDecl, + ASTContext &Ctx) override {} + bool ignoreUnsafeBufferInContainer(const SourceLocation &) const override { return false; } - bool ignoreUnsafeBufferInContainer(const SourceLocation &) const override { + bool isSafeBufferOptOut(const SourceLocation &) const override { return false; } bool ignoreUnsafeBufferInLibcCall(const SourceLocation &) const override { diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 8606227152a84..e9ca8cef07fa1 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2605,6 +2605,17 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { #endif } + void handleUnsafeUniquePtrArrayAccess(const DynTypedNode &Node, + bool IsRelatedToDecl, + ASTContext &Ctx) override { + SourceLocation Loc; + std::string Message; + + Loc = Node.get()->getBeginLoc(); + S.Diag(Loc, diag::warn_unsafe_buffer_usage_unique_ptr_array_access) + << Node.getSourceRange(); + } + bool isSafeBufferOptOut(const SourceLocation &Loc) const override { return S.PP.isSafeBufferOptOut(S.getSourceManager(), Loc); } diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-unique-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-unique-ptr.cpp new file mode 100644 index 0000000000000..789d8a2e0bb59 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-unique-ptr.cpp @@ -0,0 +1,43 @@ +// RUN: %clang_cc1 -Wno-unused-value -Wunsafe-buffer-usage -fsafe-buffer-usage-suggestions -std=c++20 -verify=expected %s + +namespace std { +inline namespace __1 { +template class unique_ptr { +public: + T &operator[](long long i) const; +}; +} // namespace __1 +} // namespace std + +int get_index() { + return 4; +} + +void basic_unique_ptr() { + std::unique_ptr p1; + int i = 2; + const int j = 3; + int k = 0; + + p1[0]; // This is allowed + + p1[k]; // expected-warning{{direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking}} + + p1[1]; // expected-warning{{direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking}} + + p1[1L]; // expected-warning{{direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking}} + + p1[1LL]; // expected-warning{{direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking}} + + p1[3 * 5]; // expected-warning{{direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking}} + + p1[i]; // expected-warning{{direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking}} + + p1[j]; // expected-warning{{direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking}} + + p1[i + 5]; // expected-warning{{direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking}} + + p1[get_index()]; // expected-warning{{direct access using operator[] on std::unique_ptr is unsafe due to lack of bounds checking}} + +} +