llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Chris Kennelly (ckennelly) <details> <summary>Changes</summary> -Wno-unsafe-buffer-usage-in-static-sized-array (https://github.com/llvm/llvm-project/commit/762a44f2c3cac98b8d3823936a620ea173cfef96) exists for code built with -fsanitize=array-bounds: subscripts on an array of known size are not reported because the sanitizer bounds-checks them. The sanitizer does not check every constant-size array, though. CodeGen's getArrayIndexingBound refuses to trust the declared size of a trailing array member that -fstrict-flex-arrays treats as a flexible array member (under the default level 0, any trailing array member), so `s->buf[idx]` in struct S { int len; int buf[16]; }; has no runtime check, yet the opt-out silenced it. Gate the opt-out on Expr::isFlexibleArrayMemberLike with the current -fstrict-flex-arrays level, the same predicate CodeGen uses. Flexible array member-like subscripts fall through to the existing static checks, so a constant index within the declared size stays quiet; the constant-offset pointer arithmetic case (https://github.com/llvm/llvm-project/commit/d6a265a477d2feba1d58f97c26ad14683fb8d1ca) is unaffected because it never relied on the sanitizer in the first place. This only affects users of the opt-out, for whom it means more warnings. --- Full diff: https://github.com/llvm/llvm-project/pull/228444.diff 5 Files Affected: - (modified) clang/docs/ReleaseNotes.md (+5) - (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+18-6) - (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array-flex-arrays.cpp (+89) - (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array-unsafe.cpp (+11) - (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array.cpp (+10) ``````````diff diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index bf190df9769ddf..24fc0aeff24e52 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -502,6 +502,11 @@ features cannot lower the translation-unit ABI level; for pointer arithmetic on statically-sized arrays when the offset is a non-negative constant within the array bounds. +- `-Wno-unsafe-buffer-usage-in-static-sized-array` no longer suppresses warnings + for subscripts on a trailing array member that `-fstrict-flex-arrays` treats + as a flexible array member, since `-fsanitize=array-bounds` does not check + those accesses. + - `-Wc++98-compat` now diagnoses explicit conversion functions in C++20 and later, matching the behavior in C++11 through C++17. (#GH161689) diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 9a4269acb1cdb8..28704008f38569 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -767,6 +767,21 @@ static bool isSafeStringViewTwoParamConstruct(const CXXConstructExpr &Node, return false; // Default to unsafe } +// Returns true iff `Node` subscripts an array whose size is known at the +// access, so that `-fsanitize=array-bounds` bounds-checks it. This is what +// `-Wno-unsafe-buffer-usage-in-static-sized-array` opts out of reporting, and +// it mirrors `getArrayIndexingBound` in CodeGen: a trailing array member that +// `-fstrict-flex-arrays` treats as a flexible array member is not checked +// because its declared size is not trusted. +static bool isSubscriptOnSizedArray(const ArraySubscriptExpr &Node, + const ASTContext &Ctx) { + const Expr *Base = Node.getBase()->IgnoreParenImpCasts(); + if (!isa<ConstantArrayType>(Base->getType()->getUnqualifiedDesugaredType())) + return false; + return !Base->isFlexibleArrayMemberLike( + Ctx, Ctx.getLangOpts().getStrictFlexArraysLevel()); +} + static bool isSafeArraySubscript(const ArraySubscriptExpr &Node, const ASTContext &Ctx, const bool IgnoreStaticSizedArrays) { @@ -777,6 +792,9 @@ static bool isSafeArraySubscript(const ArraySubscriptExpr &Node, // already duplicated // - call both from Sema and from here + if (IgnoreStaticSizedArrays && isSubscriptOnSizedArray(Node, Ctx)) + return true; + uint64_t limit; if (const auto *CATy = dyn_cast<ConstantArrayType>(Node.getBase() @@ -791,12 +809,6 @@ static bool isSafeArraySubscript(const ArraySubscriptExpr &Node, return false; } - if (IgnoreStaticSizedArrays) { - // If we made it here, it means a size was found for the var being accessed - // (either string literal or array). If it's fixed size, we can ignore it. - return true; - } - Expr::EvalResult EVResult; const Expr *IndexExpr = Node.getIdx(); if (!IndexExpr->isValueDependent() && diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array-flex-arrays.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array-flex-arrays.cpp new file mode 100644 index 00000000000000..33098da185c27b --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array-flex-arrays.cpp @@ -0,0 +1,89 @@ +// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \ +// RUN: -Wno-unsafe-buffer-usage-in-static-sized-array \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -fstrict-flex-arrays=0 -verify=expected,level0,level01,level012 %s +// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \ +// RUN: -Wno-unsafe-buffer-usage-in-static-sized-array \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -fstrict-flex-arrays=1 -verify=expected,level01,level012 %s +// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \ +// RUN: -Wno-unsafe-buffer-usage-in-static-sized-array \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -fstrict-flex-arrays=2 -verify=expected,level012 %s +// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \ +// RUN: -Wno-unsafe-buffer-usage-in-static-sized-array \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -fstrict-flex-arrays=3 -verify=expected %s + +// -Wno-unsafe-buffer-usage-in-static-sized-array exists for code built with +// -fsanitize=array-bounds, which bounds-checks subscripts on arrays of known +// size. The sanitizer does not trust the declared size of a trailing array +// member that -fstrict-flex-arrays treats as a flexible array member, so the +// opt-out must not silence accesses to those either. + +struct Zero { + int len; + int buf[0]; +}; + +struct One { + int len; + int buf[1]; +}; + +struct Many { + int len; + int buf[16]; +}; + +struct Incomplete { + int len; + int buf[]; +}; + +struct NotTrailing { + int buf[16]; + int len; +}; + +union U { + int x; + int buf[1]; +}; + +void zero(Zero *z, unsigned idx) { + z->buf[idx] = 0; // level012-warning{{unsafe buffer access}} +} + +void one(One *o, unsigned idx) { + o->buf[idx] = 0; // level01-warning{{unsafe buffer access}} + // The struct hack: a constant index past the declared size. + o->buf[1] = 0; // level01-warning{{unsafe buffer access}} +} + +void many(Many *m, unsigned idx) { + m->buf[idx] = 0; // level0-warning{{unsafe buffer access}} + m->buf[3] = 0; // a constant index within the declared size is always safe + m->buf[20] = 0; // level0-warning{{unsafe buffer access}} +} + +void incomplete(Incomplete *i, unsigned idx) { + i->buf[idx] = 0; // expected-warning{{unsafe buffer access}} +} + +void not_trailing(NotTrailing *n, unsigned idx) { + n->buf[idx] = 0; +} + +void union_member(U *u, unsigned idx) { + u->buf[idx] = 0; // level01-warning{{unsafe buffer access}} +} + +struct Method { + int len; + int buf[16]; + + void set(unsigned idx) { + buf[idx] = 0; // level0-warning{{unsafe buffer access}} + } +}; diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array-unsafe.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array-unsafe.cpp index 1165c586994562..7152146b2887ad 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array-unsafe.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array-unsafe.cpp @@ -13,3 +13,14 @@ void unsafe_pointer_arithmetic(int idx) { int *u4 = buffer + idx; // expected-note {{used in pointer arithmetic here}} } + +struct Trailing { + int len; + int buffer[10]; +}; + +// A trailing array member is a flexible array member under the default +// -fstrict-flex-arrays=0, so -fsanitize=array-bounds does not check it. +void unsafe_trailing_member(Trailing *t, int idx) { + t->buffer[idx] = 0; // expected-warning {{unsafe buffer access}} +} diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array.cpp index 9bc49525efdb97..4785096cc5cd2f 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array.cpp @@ -21,6 +21,16 @@ struct Foo { void foo2(Foo &f, unsigned idx) { f.member_buffer[idx] = 0; } +struct Trailing { + int len; + int buffer[10]; +}; + +// The trailing member is a flexible array member under the default +// -fstrict-flex-arrays=0 (see the -flex-arrays.cpp test), but a constant index +// within its declared size is safe regardless. +void trailing_constant_idx(Trailing *t) { t->buffer[9] = 0; } + void constant_idx_safe(unsigned idx) { int buffer[10]; buffer[9] = 0; `````````` </details> https://github.com/llvm/llvm-project/pull/228444 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
