From 777eb4bcfc3265359edb7c979d3e5ac699ad4641 Mon Sep 17 00:00:00 2001 From: MalavikaSamak Date: Wed, 19 Apr 2023 15:52:12 -0700 Subject: [PATCH] [-Wunsafe-buffer-usage] Handle unevaluated contexts that contain unsafe buffer usages This patch handles unevaluated contexts to ensure no warnings are produced by the machinery for buffer access made within an unevaluated contexts. However, such accesses must be considered by a FixableGadget and produce the necessary fixits. Reviewed by: NoQ, ziqingluo-90, jkorous Differential revision: https://reviews.llvm.org/D144905 --- clang/lib/Analysis/UnsafeBufferUsage.cpp | 88 +++++++++++++++++----- ...afe-buffer-usage-fixits-unevaluated-context.cpp | 79 +++++++++++++++++++ ...fe-buffer-usage-warning-unevaluated-context.cpp | 50 ++++++++++++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp | 6 -- 4 files changed, 198 insertions(+), 25 deletions(-) create mode 100644 clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp create mode 100644 clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 9908149..7a432ea 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -35,9 +35,10 @@ public: MatchDescendantVisitor(const internal::DynTypedMatcher *Matcher, internal::ASTMatchFinder *Finder, internal::BoundNodesTreeBuilder *Builder, - internal::ASTMatchFinder::BindKind Bind) + internal::ASTMatchFinder::BindKind Bind, + const bool ignoreUnevaluatedContext) : Matcher(Matcher), Finder(Finder), Builder(Builder), Bind(Bind), - Matches(false) {} + Matches(false), ignoreUnevaluatedContext(ignoreUnevaluatedContext) {} // Returns true if a match is found in a subtree of `DynNode`, which belongs // to the same callable of `DynNode`. @@ -70,6 +71,48 @@ public: return VisitorBase::TraverseDecl(Node); } + bool TraverseGenericSelectionExpr(GenericSelectionExpr *Node) { + // These are unevaluated, except the result expression. + if(ignoreUnevaluatedContext) + return TraverseStmt(Node->getResultExpr()); + return VisitorBase::TraverseGenericSelectionExpr(Node); + } + + bool TraverseUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *Node) { + // Unevaluated context. + if(ignoreUnevaluatedContext) + return true; + return VisitorBase::TraverseUnaryExprOrTypeTraitExpr(Node); + } + + bool TraverseTypeOfExprTypeLoc(TypeOfExprTypeLoc Node) { + // Unevaluated context. + if(ignoreUnevaluatedContext) + return true; + return VisitorBase::TraverseTypeOfExprTypeLoc(Node); + } + + bool TraverseDecltypeTypeLoc(DecltypeTypeLoc Node) { + // Unevaluated context. + if(ignoreUnevaluatedContext) + return true; + return VisitorBase::TraverseDecltypeTypeLoc(Node); + } + + bool TraverseCXXNoexceptExpr(CXXNoexceptExpr *Node) { + // Unevaluated context. + if(ignoreUnevaluatedContext) + return true; + return VisitorBase::TraverseCXXNoexceptExpr(Node); + } + + bool TraverseCXXTypeidExpr(CXXTypeidExpr *Node) { + // Unevaluated context. + if(ignoreUnevaluatedContext) + return true; + return VisitorBase::TraverseCXXTypeidExpr(Node); + } + bool TraverseStmt(Stmt *Node, DataRecursionQueue *Queue = nullptr) { if (!Node) return true; @@ -111,6 +154,7 @@ private: internal::BoundNodesTreeBuilder ResultBindings; const internal::ASTMatchFinder::BindKind Bind; bool Matches; + bool ignoreUnevaluatedContext; }; // Because we're dealing with raw pointers, let's define what we mean by that. @@ -121,11 +165,18 @@ static auto hasPointerType() { static auto hasArrayType() { return hasType(hasCanonicalType(arrayType())); } - -AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher, innerMatcher) { + +AST_MATCHER_P(Stmt, forEachDescendantEvaluatedStmt, internal::Matcher, innerMatcher) { const DynTypedMatcher &DTM = static_cast(innerMatcher); - MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All); + MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All, true); + return Visitor.findMatch(DynTypedNode::create(Node)); +} + +AST_MATCHER_P(Stmt, forEachDescendantStmt, internal::Matcher, innerMatcher) { + const DynTypedMatcher &DTM = static_cast(innerMatcher); + + MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All, false); return Visitor.findMatch(DynTypedNode::create(Node)); } @@ -870,32 +921,31 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler) { // clang-format off M.addMatcher( - stmt(forEveryDescendant( - eachOf( + stmt(eachOf( // A `FixableGadget` matcher and a `WarningGadget` matcher should not disable // each other (they could if they were put in the same `anyOf` group). // We also should make sure no two `FixableGadget` (resp. `WarningGadget`) matchers // match for the same node, so that we can group them // in one `anyOf` group (for better performance via short-circuiting). - stmt(eachOf( + forEachDescendantStmt(stmt(eachOf( #define FIXABLE_GADGET(x) \ x ## Gadget::matcher().bind(#x), #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" - // Also match DeclStmts because we'll need them when fixing - // their underlying VarDecls that otherwise don't have - // any backreferences to DeclStmts. - declStmt().bind("any_ds") - )), - stmt(anyOf( + // In parallel, match all DeclRefExprs so that to find out + // whether there are any uncovered by gadgets. + declRefExpr(anyOf(hasPointerType(), hasArrayType()), to(varDecl())).bind("any_dre") + ))), + forEachDescendantEvaluatedStmt(stmt(anyOf( // Add Gadget::matcher() for every gadget in the registry. #define WARNING_GADGET(x) \ allOf(x ## Gadget::matcher().bind(#x), notInSafeBufferOptOut(&Handler)), #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" - // In parallel, match all DeclRefExprs so that to find out - // whether there are any uncovered by gadgets. - declRefExpr(anyOf(hasPointerType(), hasArrayType()), to(varDecl())).bind("any_dre") - ))) - )), + // Also match DeclStmts because we'll need them when fixing + // their underlying VarDecls that otherwise don't have + // any backreferences to DeclStmts. + declStmt().bind("any_ds") + )) + ))), &CB ); // clang-format on diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp new file mode 100644 index 0000000..10d2e3e --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp @@ -0,0 +1,79 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsyntax-only %s 2>&1 | FileCheck %s + +namespace std { + class type_info; + class bad_cast; + class bad_typeid; +} +using size_t = __typeof(sizeof(int)); +void *malloc(size_t); + +void foo(...); +int bar(int *ptr); + +void uneval_context_fix_pointer_dereference() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + typeid(foo(*p)); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:15}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"[0]" + _Generic(*p, int: 2, float: 3); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:12-[[@LINE-1]]:13}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"[0]" +} + +void uneval_context_fix_pointer_array_access() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + typeid(foo(p[5])); + _Generic(p[2], int: 2, float: 3); +} + +void uneval_context_fix_pointer_reference() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + typeid(bar(p)); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:15-[[@LINE-1]]:15}:".data()" +} + +// The FixableGagdtes are not working in the following scenarios: +// 1. sizeof(DRE) +// 2. typeid(DRE) +// 3. __typeof(DRE) +// 4. _Generic(expr, type_1: DRE, type_2:) +// 5. decltype(DRE) var = y; +// 6. noexcept(DRE); +// This is becauste the UPC and ULC context matchers do not handle these contexts +// and almost all FixableGagdets currently depend on these matchers. + +// FIXME: Emit fixits for each of the below use. +void uneval_context_fix_pointer_dereference_not_handled() { + auto p = new int[10]; + int tmp = p[5]; + + foo(sizeof(*p), sizeof(decltype(*p))); + __typeof(*p) x; + int *q = (int *)malloc(sizeof(*p)); + int y = sizeof(*p); + __is_pod(__typeof(*p)); + __is_trivially_constructible(__typeof(*p), decltype(*p)); + _Generic(*p, int: 2, float: 3); + _Generic(1, int: *p, float: 3); + _Generic(1, int: 2, float: *p); + decltype(*p) var = y; + noexcept(*p); + typeid(*p); +} + diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp new file mode 100644 index 0000000..b3bcfa8 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp @@ -0,0 +1,50 @@ +// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s + +// RUN: %clang -x c++ -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 +void foo(...); // let arguments of `foo` to hold testing expressions +#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){} + +void uneval_context_fix() { + auto p = new int[10]; // expected-warning{{'p' is an unsafe pointer used for buffer access}} + + // Warn on the following DREs + _Generic(1, int: p[2], float: 3); // expected-note{{used in buffer access here}} + + // Do not warn for following DREs + auto q = new int[10]; + foo(sizeof(q[1]), // no-note + sizeof(decltype(q[1]))); // no-note + __typeof(q[5]) x; // no-note + int *r = (int *)malloc(sizeof(q[5])); // no-note + int y = sizeof(q[5]); // no-note + __is_pod(__typeof(q[5])); // no-note + __is_trivially_constructible(__typeof(q[5]), decltype(q[5])); // no-note + _Generic(q[1], int: 2, float: 3); // no-note + _Generic(1, int: 2, float: q[3]); // no-note + decltype(q[2]) var = y; // no-note + noexcept(q[2]); // no-note + typeid(q[3]); // no-note +} +#endif diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp index 783c8ea..62aeb1c 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp @@ -102,12 +102,6 @@ void testArraySubscriptsWithAuto(int *p, int **pp) { foo(ap4[1]); // expected-note{{used in buffer access here}} } -//TODO: do not warn for unevaluated context -void testUnevaluatedContext(int * p) {// expected-warning{{'p' is an unsafe pointer used for buffer access}} - foo(sizeof(p[1]), // expected-note{{used in buffer access here}} - sizeof(decltype(p[1]))); // expected-note{{used in buffer access here}} -} - void testQualifiedParameters(const int * p, const int * const q, const int a[10], const int b[10][10]) { // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}} // expected-warning@-2{{'q' is an unsafe pointer used for buffer access}} -- 2.7.4