llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: John Paul Jepko (jpjepko) <details> <summary>Changes</summary> `-Wstringop-overread` introduced in PR #<!-- -->183004 crashes when a memory function is called on `&x` where `x` is a static constexpr local inside a template, that is value-dependent but not instantiation-dependent (which happens when taking the address of something that is templated but not a local-storage variable). The checks for value and type dependency were replaced with checks only to instantiation dependency during the refactor, which caused the assertion and crash. Reland #<!-- -->183004 (reverted in #<!-- -->207840) and add fix to bring back these checks in `checkFortifiedBuiltinMemoryFunction` and add value dependence checks in `checkSourceOverread` so we don't crash when constant-evaluating these operands. Add regression test for the crash. --- Patch is 32.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/208012.diff 13 Files Affected: - (modified) clang/docs/ReleaseNotes.md (+3) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4) - (modified) clang/lib/Sema/SemaChecking.cpp (+165-82) - (modified) clang/test/AST/ByteCode/builtin-functions.cpp (+10-10) - (modified) clang/test/Analysis/bstring.c (+4) - (modified) clang/test/Analysis/malloc.c (+1) - (modified) clang/test/Analysis/pr22954.c (+1-1) - (modified) clang/test/Sema/builtin-memcpy.c (+2-1) - (modified) clang/test/Sema/builtin-object-size.c (+1-1) - (modified) clang/test/Sema/warn-fortify-source.c (+5-3) - (added) clang/test/Sema/warn-stringop-overread.c (+189) - (modified) clang/test/SemaCXX/warn-memset-bad-sizeof.cpp (+1-1) - (modified) compiler-rt/test/asan/TestCases/Windows/issue64990.cpp (+1-1) ``````````diff diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index 556e4ce3d0161..4e4e0cc5ddc6e 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -689,6 +689,9 @@ latest release, please see the [Clang Web Site](https://clang.llvm.org) or the - Clang now rejects inline asm constraints and clobbers that contain an embedded null character, instead of silently truncating them. (#GH173900) +- Added `-Wstringop-overread` to warn when `memcpy`, `memmove`, `memcmp`, + and related builtins read more bytes than the source buffer size (#GH83728). + - Diagnostics for the C++11 range-based for statement now report the correct iterator type in notes for invalid iterator types. diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 38d9e4046d3a5..05f49a4dbd1ca 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -969,6 +969,10 @@ def warn_fortify_source_size_mismatch : Warning< "'%0' size argument is too large; destination buffer has size %1," " but size argument is %2">, InGroup<FortifySource>; +def warn_stringop_overread + : Warning<"'%0' reading %1 byte%s1 from a region of size %2">, + InGroup<DiagGroup<"stringop-overread">>; + def warn_fortify_strlen_overflow: Warning< "'%0' will always overflow; destination buffer has size %1," " but the source string has length %2 (including NUL byte)">, diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index d52c12670a57b..5ae83dffece2e 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -1147,36 +1147,23 @@ static bool ProcessFormatStringLiteral(const Expr *FormatExpr, return false; } -void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, - CallExpr *TheCall) { - if (TheCall->isValueDependent() || TheCall->isTypeDependent() || - isConstantEvaluatedContext()) - return; - - bool UseDABAttr = false; - const FunctionDecl *UseDecl = FD; - - const auto *DABAttr = FD->getAttr<DiagnoseAsBuiltinAttr>(); - if (DABAttr) { - UseDecl = DABAttr->getFunction(); - assert(UseDecl && "Missing FunctionDecl in DiagnoseAsBuiltin attribute!"); - UseDABAttr = true; +namespace { +/// Helper class for buffer overflow/overread checking in fortified functions. +class FortifiedBufferChecker { +public: + FortifiedBufferChecker(Sema &S, FunctionDecl *FD, CallExpr *TheCall) + : S(S), TheCall(TheCall), FD(FD), + DABAttr(FD ? FD->getAttr<DiagnoseAsBuiltinAttr>() : nullptr) { + const TargetInfo &TI = S.getASTContext().getTargetInfo(); + SizeTypeWidth = TI.getTypeWidth(TI.getSizeType()); } - unsigned BuiltinID = UseDecl->getBuiltinID(/*ConsiderWrappers=*/true); - - if (!BuiltinID) - return; - - const TargetInfo &TI = getASTContext().getTargetInfo(); - unsigned SizeTypeWidth = TI.getTypeWidth(TI.getSizeType()); - - auto TranslateIndex = [&](unsigned Index) -> std::optional<unsigned> { + std::optional<unsigned> TranslateIndex(unsigned Index) { // If we refer to a diagnose_as_builtin attribute, we need to change the // argument index to refer to the arguments of the called function. Unless // the index is out of bounds, which presumably means it's a variadic // function. - if (!UseDABAttr) + if (!DABAttr) return Index; unsigned DABIndices = DABAttr->argIndices_size(); unsigned NewIndex = Index < DABIndices @@ -1185,25 +1172,25 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, if (NewIndex >= TheCall->getNumArgs()) return std::nullopt; return NewIndex; - }; + } - auto ComputeExplicitObjectSizeArgument = - [&](unsigned Index) -> std::optional<llvm::APSInt> { + std::optional<llvm::APSInt> + ComputeExplicitObjectSizeArgument(unsigned Index) { std::optional<unsigned> IndexOptional = TranslateIndex(Index); if (!IndexOptional) return std::nullopt; unsigned NewIndex = *IndexOptional; Expr::EvalResult Result; Expr *SizeArg = TheCall->getArg(NewIndex); - if (!SizeArg->EvaluateAsInt(Result, getASTContext())) + if (!SizeArg->EvaluateAsInt(Result, S.getASTContext())) return std::nullopt; llvm::APSInt Integer = Result.Val.getInt(); - Integer.setIsUnsigned(true); + assert(Integer.isUnsigned() && + "size arg should be unsigned after implicit conversion to size_t"); return Integer; - }; + } - auto ComputeSizeArgument = - [&](unsigned Index) -> std::optional<llvm::APSInt> { + std::optional<llvm::APSInt> ComputeSizeArgument(unsigned Index) { // If the parameter has a pass_object_size attribute, then we should use its // (potentially) more strict checking mode. Otherwise, conservatively assume // type 0. @@ -1225,15 +1212,14 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, const Expr *ObjArg = TheCall->getArg(NewIndex); if (std::optional<uint64_t> ObjSize = - ObjArg->tryEvaluateObjectSize(getASTContext(), BOSType)) { + ObjArg->tryEvaluateObjectSize(S.getASTContext(), BOSType)) { // Get the object size in the target's size_t width. return llvm::APSInt::getUnsigned(*ObjSize).extOrTrunc(SizeTypeWidth); } return std::nullopt; - }; + } - auto ComputeStrLenArgument = - [&](unsigned Index) -> std::optional<llvm::APSInt> { + std::optional<llvm::APSInt> ComputeStrLenArgument(unsigned Index) { std::optional<unsigned> IndexOptional = TranslateIndex(Index); if (!IndexOptional) return std::nullopt; @@ -1242,33 +1228,97 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, const Expr *ObjArg = TheCall->getArg(NewIndex); if (std::optional<uint64_t> Result = - ObjArg->tryEvaluateStrLen(getASTContext())) { + ObjArg->tryEvaluateStrLen(S.getASTContext())) { // Add 1 for null byte. return llvm::APSInt::getUnsigned(*Result + 1).extOrTrunc(SizeTypeWidth); } return std::nullopt; - }; + } + + unsigned getSizeTypeWidth() const { return SizeTypeWidth; } + + unsigned getBuiltinID() const { + const FunctionDecl *UseDecl = FD; + if (DABAttr) { + UseDecl = DABAttr->getFunction(); + assert(UseDecl && "Missing FunctionDecl in DiagnoseAsBuiltin attribute!"); + } + return UseDecl->getBuiltinID(/*ConsiderWrappers=*/true); + } + + /// Return function name after stripping __builtin_ and _chk affixes. + std::string getFunctionName() const { + unsigned ID = getBuiltinID(); + if (!ID) { + // Use callee name directly if not a builtin. + const FunctionDecl *Callee = TheCall->getDirectCallee(); + assert(Callee && "expected callee"); + return Callee->getName().str(); + } + std::string Name = S.getASTContext().BuiltinInfo.getName(ID); + StringRef Ref = Name; + // Strip __builtin___*_chk or __builtin_ prefix. + if (!(Ref.consume_front("__builtin___") && Ref.consume_back("_chk"))) + Ref.consume_front("__builtin_"); + assert(!Ref.empty() && "expected non-empty function name"); + return Ref.str(); + } + + /// Check for source buffer overread in memory functions. + void checkSourceOverread(unsigned SrcArgIdx, unsigned SizeArgIdx) { + if (S.isConstantEvaluatedContext()) + return; + + const Expr *SrcArg = TheCall->getArg(SrcArgIdx); + const Expr *SizeArg = TheCall->getArg(SizeArgIdx); + // Need to check both value-dependence and instantiation-dependence. + if (SrcArg->isInstantiationDependent() || SrcArg->isValueDependent() || + SizeArg->isInstantiationDependent() || SizeArg->isValueDependent()) + return; + + std::optional<llvm::APSInt> CopyLen = + ComputeExplicitObjectSizeArgument(SizeArgIdx); + std::optional<llvm::APSInt> SrcBufSize = ComputeSizeArgument(SrcArgIdx); + + if (!CopyLen || !SrcBufSize) + return; + + // Warn only if copy length exceeds source buffer size. + if (llvm::APSInt::compareValues(*CopyLen, *SrcBufSize) <= 0) + return; + + S.DiagRuntimeBehavior(TheCall->getBeginLoc(), TheCall, + S.PDiag(diag::warn_stringop_overread) + << getFunctionName() << CopyLen->getZExtValue() + << SrcBufSize->getZExtValue()); + } + +private: + Sema &S; + CallExpr *TheCall; + FunctionDecl *FD; + const DiagnoseAsBuiltinAttr *DABAttr; + unsigned SizeTypeWidth; +}; +} // anonymous namespace + +void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, + CallExpr *TheCall) { + if (TheCall->isInstantiationDependent() || TheCall->isValueDependent() || + TheCall->isTypeDependent() || isConstantEvaluatedContext()) + return; + + FortifiedBufferChecker Checker(*this, FD, TheCall); + + unsigned BuiltinID = Checker.getBuiltinID(); + if (!BuiltinID) + return; + + unsigned SizeTypeWidth = Checker.getSizeTypeWidth(); std::optional<llvm::APSInt> SourceSize; std::optional<llvm::APSInt> DestinationSize; unsigned DiagID = 0; - bool IsChkVariant = false; - - auto GetFunctionName = [&]() { - std::string FunctionNameStr = - getASTContext().BuiltinInfo.getName(BuiltinID); - llvm::StringRef FunctionName = FunctionNameStr; - // Skim off the details of whichever builtin was called to produce a better - // diagnostic, as it's unlikely that the user wrote the __builtin - // explicitly. - if (IsChkVariant) { - FunctionName = FunctionName.drop_front(std::strlen("__builtin___")); - FunctionName = FunctionName.drop_back(std::strlen("_chk")); - } else { - FunctionName.consume_front("__builtin_"); - } - return FunctionName.str(); - }; switch (BuiltinID) { default: @@ -1280,8 +1330,8 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, case Builtin::BI__builtin_strcpy: case Builtin::BIstrcpy: { DiagID = diag::warn_fortify_strlen_overflow; - SourceSize = ComputeStrLenArgument(1); - DestinationSize = ComputeSizeArgument(0); + SourceSize = Checker.ComputeStrLenArgument(1); + DestinationSize = Checker.ComputeSizeArgument(0); break; } @@ -1289,9 +1339,8 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, case Builtin::BI__builtin___stpcpy_chk: case Builtin::BI__builtin___strcpy_chk: { DiagID = diag::warn_fortify_strlen_overflow; - SourceSize = ComputeStrLenArgument(1); - DestinationSize = ComputeExplicitObjectSizeArgument(2); - IsChkVariant = true; + SourceSize = Checker.ComputeStrLenArgument(1); + DestinationSize = Checker.ComputeExplicitObjectSizeArgument(2); break; } @@ -1317,14 +1366,14 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, unsigned SourceSize) { DiagID = diag::warn_fortify_scanf_overflow; unsigned Index = ArgIndex + DataIndex; - std::string FunctionName = GetFunctionName(); + std::string FunctionName = Checker.getFunctionName(); DiagRuntimeBehavior(TheCall->getArg(Index)->getBeginLoc(), TheCall, PDiag(DiagID) << FunctionName << (Index + 1) << DestSize << SourceSize); }; auto ShiftedComputeSizeArgument = [&](unsigned Index) { - return ComputeSizeArgument(Index + DataIndex); + return Checker.ComputeSizeArgument(Index + DataIndex); }; ScanfDiagnosticFormatHandler H(ShiftedComputeSizeArgument, Diagnose); const char *FormatBytes = FormatStrRef.data(); @@ -1357,10 +1406,9 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, SourceSize = llvm::APSInt::getUnsigned(H.getSizeLowerBound()) .extOrTrunc(SizeTypeWidth); if (BuiltinID == Builtin::BI__builtin___sprintf_chk) { - DestinationSize = ComputeExplicitObjectSizeArgument(2); - IsChkVariant = true; + DestinationSize = Checker.ComputeExplicitObjectSizeArgument(2); } else { - DestinationSize = ComputeSizeArgument(0); + DestinationSize = Checker.ComputeSizeArgument(0); } break; } @@ -1378,19 +1426,24 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, case Builtin::BI__builtin___memccpy_chk: case Builtin::BI__builtin___mempcpy_chk: { DiagID = diag::warn_builtin_chk_overflow; - SourceSize = ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 2); + SourceSize = + Checker.ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 2); DestinationSize = - ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 1); - IsChkVariant = true; + Checker.ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 1); + + if (BuiltinID == Builtin::BI__builtin___memcpy_chk || + BuiltinID == Builtin::BI__builtin___memmove_chk || + BuiltinID == Builtin::BI__builtin___mempcpy_chk) { + Checker.checkSourceOverread(/*SrcArgIdx=*/1, /*SizeArgIdx=*/2); + } break; } case Builtin::BI__builtin___snprintf_chk: case Builtin::BI__builtin___vsnprintf_chk: { DiagID = diag::warn_builtin_chk_overflow; - SourceSize = ComputeExplicitObjectSizeArgument(1); - DestinationSize = ComputeExplicitObjectSizeArgument(3); - IsChkVariant = true; + SourceSize = Checker.ComputeExplicitObjectSizeArgument(1); + DestinationSize = Checker.ComputeExplicitObjectSizeArgument(3); break; } @@ -1406,8 +1459,9 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, // size larger than the destination buffer though; this is a runtime abort // in _FORTIFY_SOURCE mode, and is quite suspicious otherwise. DiagID = diag::warn_fortify_source_size_mismatch; - SourceSize = ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 1); - DestinationSize = ComputeSizeArgument(0); + SourceSize = + Checker.ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 1); + DestinationSize = Checker.ComputeSizeArgument(0); break; } @@ -1422,23 +1476,52 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, case Builtin::BImempcpy: case Builtin::BI__builtin_mempcpy: { DiagID = diag::warn_fortify_source_overflow; - SourceSize = ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 1); - DestinationSize = ComputeSizeArgument(0); + SourceSize = + Checker.ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 1); + DestinationSize = Checker.ComputeSizeArgument(0); + + // Buffer overread doesn't make sense for memset/bzero. + if (BuiltinID != Builtin::BImemset && + BuiltinID != Builtin::BI__builtin_memset && + BuiltinID != Builtin::BIbzero && + BuiltinID != Builtin::BI__builtin_bzero) { + Checker.checkSourceOverread(/*SrcArgIdx=*/1, /*SizeArgIdx=*/2); + } break; } case Builtin::BIbcopy: case Builtin::BI__builtin_bcopy: { DiagID = diag::warn_fortify_source_overflow; - SourceSize = ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 1); - DestinationSize = ComputeSizeArgument(1); + SourceSize = + Checker.ComputeExplicitObjectSizeArgument(TheCall->getNumArgs() - 1); + DestinationSize = Checker.ComputeSizeArgument(1); + Checker.checkSourceOverread(/*SrcArgIdx=*/0, /*SizeArgIdx=*/2); break; } + + // memchr(buf, val, size) + case Builtin::BImemchr: + case Builtin::BI__builtin_memchr: { + Checker.checkSourceOverread(/*SrcArgIdx=*/0, /*SizeArgIdx=*/2); + return; + } + + // memcmp/bcmp(buf0, buf1, size) + // Two checks since each buffer is read + case Builtin::BImemcmp: + case Builtin::BI__builtin_memcmp: + case Builtin::BIbcmp: + case Builtin::BI__builtin_bcmp: { + Checker.checkSourceOverread(/*SrcArgIdx=*/0, /*SizeArgIdx=*/2); + Checker.checkSourceOverread(/*SrcArgIdx=*/1, /*SizeArgIdx=*/2); + return; + } case Builtin::BIsnprintf: case Builtin::BI__builtin_snprintf: case Builtin::BIvsnprintf: case Builtin::BI__builtin_vsnprintf: { DiagID = diag::warn_fortify_source_size_mismatch; - SourceSize = ComputeExplicitObjectSizeArgument(1); + SourceSize = Checker.ComputeExplicitObjectSizeArgument(1); const auto *FormatExpr = TheCall->getArg(2)->IgnoreParenImpCasts(); StringRef FormatStrRef; size_t StrLen; @@ -1462,12 +1545,12 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, FormatSize.toString(FormatSizeStr, /*Radix=*/10); DiagRuntimeBehavior(TheCall->getBeginLoc(), TheCall, PDiag(TruncationDiagID) - << GetFunctionName() << SpecifiedSizeStr - << FormatSizeStr); + << Checker.getFunctionName() + << SpecifiedSizeStr << FormatSizeStr); } } } - DestinationSize = ComputeSizeArgument(0); + DestinationSize = Checker.ComputeSizeArgument(0); const Expr *LenArg = TheCall->getArg(1)->IgnoreCasts(); const Expr *Dest = TheCall->getArg(0)->IgnoreCasts(); IdentifierInfo *FnInfo = FD->getIdentifier(); @@ -1479,7 +1562,7 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, llvm::APSInt::compareValues(*SourceSize, *DestinationSize) <= 0) return; - std::string FunctionName = GetFunctionName(); + std::string FunctionName = Checker.getFunctionName(); SmallString<16> DestinationStr; SmallString<16> SourceStr; diff --git a/clang/test/AST/ByteCode/builtin-functions.cpp b/clang/test/AST/ByteCode/builtin-functions.cpp index d6990d1725072..b14ae91c753cb 100644 --- a/clang/test/AST/ByteCode/builtin-functions.cpp +++ b/clang/test/AST/ByteCode/builtin-functions.cpp @@ -1,17 +1,17 @@ -// RUN: %clang_cc1 -Wno-string-plus-int -fexperimental-new-constant-interpreter -triple x86_64 %s -verify=expected,both -// RUN: %clang_cc1 -Wno-string-plus-int -triple x86_64 %s -verify=ref,both +// RUN: %clang_cc1 -Wno-string-plus-int -Wno-stringop-overread -fexperimental-new-constant-interpreter -triple x86_64 %s -verify=expected,both +// RUN: %clang_cc1 -Wno-string-plus-int -Wno-stringop-overread -triple x86_64 %s -verify=ref,both // -// RUN: %clang_cc1 -Wno-string-plus-int -fexperimental-new-constant-interpreter -triple i686 %s -verify=expected,both -// RUN: %clang_cc1 -Wno-string-plus-int -triple i686 %s -verify=ref,both +// RUN: %clang_cc1 -Wno-string-plus-int -Wno-stringop-overread -fexperimental-new-constant-interpreter -triple i686 %s -verify=expected,both +// RUN: %clang_cc1 -Wno-string-plus-int -Wno-stringop-overread -triple i686 %s -verify=ref,both // -// RUN: %clang_cc1 -std=c++20 -Wno-string-plus-int -fexperimental-new-constant-interpreter -triple x86_64 %s -verify=expected,both -// RUN: %clang_cc1 -std=c++20 -Wno-string-plus-int -triple x86_64 %s -verify=ref,both +// RUN: %clang_cc1 -std=c++20 -Wno-string-plus-int -Wno-stringop-overread -fexperimental-new-constant-interpreter -triple x86_64 %s -verify=expected,both +// RUN: %clang_cc1 -std=c++20 -Wno-string-plus-int -Wno-stringop-overread -triple x86_64 %s -verify=ref,both // -// RUN: %clang_cc1 -std=c++20 -Wno-string-plus-int -fexperimental-new-constant-interpreter -triple i686 %s -verify=expected,both -// RUN: %clang_cc1 -std=c++20 -Wno-string-plus-int -triple i686 %s -verify=ref,both +// RUN: %clang_cc1 -std=c++20 -Wno-string-plus-int -Wno-stringop-overread -fexperimental-new-constant-interpreter -triple i686 %s -verify=expected,both +// RUN: %clang_cc1 -std=c++20 -Wno-string-plus-int -Wno-stringop-overread -triple i686 %s -verify=ref,both // -// RUN: %clang_cc1 -triple avr -std=c++20 -Wno-string-plus-int -fexperimental-new-constant-interpreter %s -verify=expected,both -// RUN: %clang_cc1 -triple avr -std=c++20 -Wno-string-plus-int -verify=ref,both %s +// RUN: %clang_cc1 -triple avr -std=c++20 -... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/208012 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
