Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h =================================================================== --- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -41,6 +41,7 @@ // through the handler class. void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler); +static std::string UserFillPlaceHolder = "..."; } // end namespace clang #endif /* LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H */ Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -610,8 +610,120 @@ return {std::move(CB.Gadgets), std::move(CB.Tracker)}; } +// For a non-null initializer `Init` of `T *` type, this function writes +// `{Init, S}` as a part of a fix-it to output stream . The list-initializer +// `{Init, S}` belongs to a span constructor, where `Init` specifies the +// address of the data and `S` is the extent. In many cases, this function +// cannot figure out the actual extent `S`. It then will use a place holder to +// replace `S` to ask users to fill `S` in. The span object being constructed +// will have type `span`. +// +// FIXME: This function so far only works for spanify "single-level" pointers. +// When it comes to multi-level pointers, the data address is not necessarily +// identical to `Init` and the element type of the constructing span object is +// not necessarily `T`. +// +// Parameters: +// `Init` a pointer to the initializer expression +// `Ctx` a reference to the ASTContext +// `OS` the output stream where fix-it texts being written to +static void populateInitializerFixItWithSpan(const Expr *Init, + const ASTContext &Ctx, raw_ostream &OS) { + const PrintingPolicy &PP = Ctx.getPrintingPolicy(); + + // Prints the begin address of the span constructor: + OS << "{"; + Init->printPretty(OS, nullptr, PP); + OS << ", "; + + bool ExtKnown = false; // true iff the extent can be obtained + // Printers that print extent into OS and sets ExtKnown to true: + std::function PrintExpr = [&ExtKnown, &OS, &PP](const Expr *Size) { + Size->printPretty(OS, nullptr, PP); + ExtKnown = true; + }; + std::function PrintAPInt = [&ExtKnown, &OS](const APInt &Size) { + Size.print(OS, false); + ExtKnown = true; + }; + std::function PrintOne = [&ExtKnown, &OS](void) { + OS << "1"; + ExtKnown = true; + }; + + // Try to get the data extent. Break into different cases: + if (auto CxxNew = dyn_cast(Init->IgnoreImpCasts())) { + // In cases `Init` is `new T[n]` and there is no explicit cast over + // `Init`, we know that `Init` must evaluates to a pointer to `n` objects + // of `T`. So the extent is `n` unless `n` has side effects. Similar but + // simpler for `Init` is `new T`. + if (const Expr *Ext = CxxNew->getArraySize().value_or(nullptr)) { + if (!Ext->HasSideEffects(Ctx)) + PrintExpr(Ext); + } else if (!CxxNew->isArray()) + PrintOne(); + } else if (auto CArrTy = dyn_cast( + Init->IgnoreImpCasts()->getType().getTypePtr())) { + // In cases `Init` is of an array type after stripping off implicit casts, + // `PointeeTy` must besimilar to the element type of `CArrTy`, so the + // extent is the array size of `CArrTy`. Note that if the array size is + // not a constant, we cannot use it as the extent. + PrintAPInt(CArrTy->getSize()); + // FIXME: local static array may be converted to std:array. Is there any + // dependency among these fix-its? + } else { + // In cases `Init` is of the form `&Var` after stripping of implicit + // casts, where `&` is the built-in operator, `PointeeTy` must be similar + // to the type of `Var`, so the extent is 1. + if (auto AddrOfExpr = dyn_cast(Init->IgnoreImpCasts())) + if (AddrOfExpr->getOpcode() == UnaryOperatorKind::UO_AddrOf && + isa_and_present(AddrOfExpr->getSubExpr())) + PrintOne(); + // TODO: we can handle more cases, e.g., `&a[0]`, `&a`, std::addressof,... + // etc. + } + if (!ExtKnown) + OS << UserFillPlaceHolder; + OS << "}"; +} + +// For a `VarDecl` of the form `T * var (= Init)?`, this +// function generates a fix-it for the declaration, which re-declares `var` to +// be of `span` type and transforms the initializer, if present, to a span +// constructor---`span var {Init, Extent}`, where `Extent` may need the user +// to fill in. +// +// FIXME: +// For now, we only consider single level pointer types, i.e., given `T` is +// `int **`, the converted type will be `span`. +// We need to handle multi-level pointers correctly later. +// +// Parameters: +// `D` a pointer the variable declaration node +// `Ctx` a reference to the ASTContext +// Returns: +// the generated fix-it +static FixItHint fixVarDeclWithSpan(const VarDecl *D, + const ASTContext &Ctx) { + const QualType &T = D->getType().getDesugaredType(Ctx); + const QualType &SpanEltT = T->getPointeeType(); + assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!"); + + SmallString<32> Replacement; + raw_svector_ostream OS(Replacement); + + // Simply make `D` to be of span type: + OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName(); + if (const Expr *Init = D->getInit()) + populateInitializerFixItWithSpan(Init, Ctx, OS); + + return FixItHint::CreateReplacement( + SourceRange{D->getInnerLocStart(), D->getEndLoc()}, OS.str()); +} + static FixItList fixVariableWithSpan(const VarDecl *VD, - const DeclUseTracker &Tracker) { + const DeclUseTracker &Tracker, + const ASTContext &Ctx) { const DeclStmt *DS = Tracker.lookupDecl(VD); assert(DS && "Fixing non-local variables not implemented yet!"); assert(DS->getSingleDecl() == VD && @@ -621,27 +733,17 @@ // and the '*' are spread around the place. (void)DS; - QualType T = VD->getType()->getPointeeType(); - assert(!T.isNull() && "Trying to fix a non-pointer type variable!"); - - SmallString<32> Replacement; - raw_svector_ostream OS(Replacement); - OS << "std::span<" << T.getAsString() << ", ...>"; - if (!VD->getType()->getAs()) - OS << " "; - - FixItHint Fix = FixItHint::CreateReplacement( - SourceRange{VD->getTypeSpecStartLoc(), VD->getTypeSpecEndLoc()}, - OS.str()); - + // FIXME: handle cases where DS has multiple declarations + FixItHint Fix = fixVarDeclWithSpan(VD, Ctx); return {Fix}; } static FixItList fixVariable(const VarDecl *VD, Strategy::Kind K, - const DeclUseTracker &Tracker) { + const DeclUseTracker &Tracker, + const ASTContext &Ctx) { switch (K) { case Strategy::Kind::Span: - return fixVariableWithSpan(VD, Tracker); + return fixVariableWithSpan(VD, Tracker, Ctx); case Strategy::Kind::Iterator: case Strategy::Kind::Array: case Strategy::Kind::Vector: @@ -683,6 +785,7 @@ } Strategy S; + const ASTContext &Ctx = D->getASTContext(); for (const auto &Item : Map) { const VarDecl *VD = Item.first; @@ -724,7 +827,7 @@ if (Fixes) { // If we reach this point, the strategy is applicable. // Add a fixit for the variable itself and emit all fixes. - FixItList VarF = fixVariable(VD, S.lookup(VD), Tracker); + FixItList VarF = fixVariable(VD, S.lookup(VD), Tracker, Ctx); for (auto &&Fixit : VarF) Fixes->push_back(std::move(Fixit)); @@ -740,3 +843,4 @@ } } } + Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp =================================================================== --- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp @@ -3,16 +3,71 @@ // RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp // RUN: grep -v CHECK %t.cpp | FileCheck %s +void foo(...); +typedef int * Int_ptr_t; +typedef int Int_t; + void local_array_subscript_simple() { -// CHECK: std::span p = new int[10]; +// CHECK: std::span p{new int [10], 10}; // CHECK: p[5] = 5; int *p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}} p[5] = 5; + +// CHECK: std::span q{new int [10], 10}; +// CHECK: std::span x{new int [10], 10}; +// CHECK: std::span y{new int, 1}; +// CHECK: std::span z{new int [10], 10}; +// CHECK: std::span w{new Int_t [10], 10}; +// CHECK: foo(q[5], x[5], y[5], z[5], w[5]); + const int *q = new int[10]; // expected-warning{{variable 'q' participates in unchecked buffer operations}} + Int_ptr_t x = new int[10]; // expected-warning{{variable 'x' participates in unchecked buffer operations}} + Int_ptr_t y = new int; // expected-warning{{variable 'y' participates in unchecked buffer operations}} + Int_t * z = new int[10]; // expected-warning{{variable 'z' participates in unchecked buffer operations}} + Int_t * w = new Int_t[10]; // expected-warning{{variable 'w' participates in unchecked buffer operations}} + foo(q[5], x[5], y[5], z[5], w[5]); + // y[5] will crash after being span } void local_array_subscript_auto() { -// CHECK: std::span p = new int[10]; +// CHECK: std::span p{new int [10], 10}; // CHECK: p[5] = 5; auto p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}} p[5] = 5; } + +void local_array_subscript_variable_extent() { + int n = 10; +// CHECK: std::span p{new int [n], n}; +// CHECK: std::span q{new int [n++], ...}; +// CHECK: foo(p[5], q[5]); + int *p = new int[n]; // expected-warning{{variable 'p' participates in unchecked buffer operations}} + // If the extent expression does not have a constant value, we cannot fill the extent for users... + int *q = new int[n++]; // expected-warning{{variable 'q' participates in unchecked buffer operations}} + foo(p[5], q[5]); +} + + +void local_ptr_to_array() { + int n = 10; + int a[10]; + int b[n]; // If the extent expression does not have a constant value, we cannot fill the extent for users... +// CHECK: std::span p{a, 10}; +// CHECK: std::span q{b, ...}; +// CHECK: foo(p[5], q[5]); + int *p = a; // expected-warning{{variable 'p' participates in unchecked buffer operations}} + int *q = b; // expected-warning{{variable 'q' participates in unchecked buffer operations}} + foo(p[5], q[5]); +} + +void local_ptr_addrof_init() { + int a[10]; + int var; +// CHECK: std::span p{&a, 1}; +// CHECK: std::span q{&var, 1}; +// CHECK: foo(p[5], q[5]); + int (*p)[10] = &a; // expected-warning{{variable 'p' participates in unchecked buffer operations}} + int * q = &var; // expected-warning{{variable 'q' participates in unchecked buffer operations}} + // These two expressions involve unsafe buffer accesses, which will + // crash at runtime after applying the fix-it, + foo(p[5], q[5]); +} Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp =================================================================== --- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp +++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp @@ -69,19 +69,19 @@ auto ap1 = a; // expected-warning{{variable 'ap1' participates in unchecked buffer operations}} - foo(ap1[1]); // expected-warning{{unchecked operation on raw buffer in expression}} + foo(ap1[1]); auto ap2 = p; // expected-warning{{variable 'ap2' participates in unchecked buffer operations}} - foo(ap2[1]); // expected-warning{{unchecked operation on raw buffer in expression}} + foo(ap2[1]); - auto ap3 = pp; + auto ap3 = pp; // expected-warning{{variable 'ap3' participates in unchecked buffer operations}} - foo(ap3[1][1]); // expected-warning2{{unchecked operation on raw buffer in expression}} + foo(ap3[1][1]); // expected-warning{{unchecked operation on raw buffer in expression}} auto ap4 = *pp; // expected-warning{{variable 'ap4' participates in unchecked buffer operations}} - foo(ap4[1]); // expected-warning{{unchecked operation on raw buffer in expression}} + foo(ap4[1]); } void testQualifiedParameters(const int * p, const int * const q,