llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Endre Fülöp (gamesh411) <details> <summary>Changes</summary> …tedOrUnsafeBuf…" (#<!-- -->176603) The bolt buildbot failure for aarch64 seems unrelated. This reverts commit 17ff9b3c67ab62f77b2dd5cba98fa0011f14d9a2. --- Full diff: https://github.com/llvm/llvm-project/pull/177376.diff 11 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp (+2-20) - (modified) clang/docs/analyzer/checkers.rst (+13) - (added) clang/include/clang/Analysis/AnnexKDetection.h (+40) - (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+19-6) - (added) clang/lib/Analysis/AnnexKDetection.cpp (+43) - (modified) clang/lib/Analysis/CMakeLists.txt (+1) - (modified) clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp (+76-12) - (modified) clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (+19) - (modified) clang/test/Analysis/Inputs/system-header-simulator.h (+1) - (modified) clang/test/Analysis/analyzer-config.c (+1) - (added) clang/test/Analysis/security-deprecated-buffer-handling-report-modes.c (+40) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp index 6694356a2be98..52d11309c7842 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp @@ -10,6 +10,7 @@ #include "../utils/OptionsUtils.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Analysis/AnnexKDetection.h" #include "clang/Lex/PPCallbacks.h" #include "clang/Lex/Preprocessor.h" #include <cassert> @@ -113,26 +114,7 @@ static bool isAnnexKAvailable(std::optional<bool> &CacheVar, Preprocessor *PP, if (CacheVar.has_value()) return *CacheVar; - if (!LO.C11) - // TODO: How is "Annex K" available in C++ mode? - return (CacheVar = false).value(); - - assert(PP && "No Preprocessor registered."); - - if (!PP->isMacroDefined("__STDC_LIB_EXT1__") || - !PP->isMacroDefined("__STDC_WANT_LIB_EXT1__")) - return (CacheVar = false).value(); - - const auto *MI = - PP->getMacroInfo(PP->getIdentifierInfo("__STDC_WANT_LIB_EXT1__")); - if (!MI || MI->tokens_empty()) - return (CacheVar = false).value(); - - const Token &T = MI->tokens().back(); - if (!T.isLiteral() || !T.getLiteralData()) - return (CacheVar = false).value(); - - CacheVar = StringRef(T.getLiteralData(), T.getLength()) == "1"; + CacheVar = analysis::isAnnexKAvailable(PP, LO); return CacheVar.value(); } diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index a2774de4a189b..e449593e95d21 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -1785,6 +1785,19 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C) strncpy(buf, "a", 1); // warn } +The ``ReportMode`` option controls when warnings are reported: + +* ``all``: Reports all unsafe functions regardless of C standard or Annex K availability. Useful for security auditing and vulnerability scanning. + +* ``actionable``: Only reports when Annex K is available (C11 with ``__STDC_LIB_EXT1__`` and ``__STDC_WANT_LIB_EXT1__=1``). + +* ``c11-only``: Reports when C11 standard is enabled (does not take Annex K availability into account). + +To set this option, use: +``-analyzer-config security.insecureAPI.DeprecatedOrUnsafeBufferHandling:ReportMode=all`` + +By default, this option is set to *c11-only*. + .. _security-MmapWriteExec: security.MmapWriteExec (C) diff --git a/clang/include/clang/Analysis/AnnexKDetection.h b/clang/include/clang/Analysis/AnnexKDetection.h new file mode 100644 index 0000000000000..5114f25dfa719 --- /dev/null +++ b/clang/include/clang/Analysis/AnnexKDetection.h @@ -0,0 +1,40 @@ +//==- AnnexKDetection.h - Annex K availability detection ---------*- C++ -*-==// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file provides utilities for detecting C11 Annex K (Bounds-checking +// interfaces) availability. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_ANALYSIS_ANNEXKDETECTION_H +#define LLVM_CLANG_ANALYSIS_ANNEXKDETECTION_H + +namespace clang { +class Preprocessor; +class LangOptions; +} // namespace clang + +namespace clang::analysis { + +/// Calculates whether Annex K is available for the current translation unit +/// based on the macro definitions and the language options. +/// +/// Annex K (Bounds-checking interfaces) is available when: +/// 1. C11 standard is enabled +/// 2. __STDC_LIB_EXT1__ macro is defined (indicates library support) +/// 3. __STDC_WANT_LIB_EXT1__ macro is defined and equals "1" (indicates user +/// opt-in) +/// +/// \param PP The preprocessor instance to check macro definitions. +/// \param LO The language options to check C11 standard. +/// \returns true if Annex K is available, false otherwise. +[[nodiscard]] bool isAnnexKAvailable(Preprocessor *PP, const LangOptions &LO); + +} // namespace clang::analysis + +#endif // LLVM_CLANG_ANALYSIS_ANNEXKDETECTION_H diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index e1662e0792e69..91738e6a29664 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -907,12 +907,25 @@ def UncheckedReturn : Checker<"UncheckedReturn">, Dependencies<[SecuritySyntaxChecker]>, Documentation<HasDocumentation>; -def DeprecatedOrUnsafeBufferHandling : - Checker<"DeprecatedOrUnsafeBufferHandling">, - HelpText<"Warn on uses of unsecure or deprecated buffer manipulating " - "functions">, - Dependencies<[SecuritySyntaxChecker]>, - Documentation<HasDocumentation>; +def DeprecatedOrUnsafeBufferHandling + : Checker<"DeprecatedOrUnsafeBufferHandling">, + HelpText<"Warn on uses of unsecure or deprecated buffer manipulating " + "functions">, + Dependencies<[SecuritySyntaxChecker]>, + CheckerOptions< + [CmdLineOption< + String, "ReportMode", + "Controls when warnings are reported. \"all\" reports all " + "unsafe functions regardless of C standard or Annex K " + "availability. \"actionable\" only reports when Annex K is " + "available (C11 with __STDC_LIB_EXT1__ and " + "__STDC_WANT_LIB_EXT1__=1). \"c11-only\" reports when C11 " + "standard is enabled (does not take Annex K availability into " + "account).", + "c11-only", + Released>, + ]>, + Documentation<HasDocumentation>; def decodeValueOfObjCType : Checker<"decodeValueOfObjCType">, HelpText<"Warn on uses of the '-decodeValueOfObjCType:at:' method">, diff --git a/clang/lib/Analysis/AnnexKDetection.cpp b/clang/lib/Analysis/AnnexKDetection.cpp new file mode 100644 index 0000000000000..46da677dc1014 --- /dev/null +++ b/clang/lib/Analysis/AnnexKDetection.cpp @@ -0,0 +1,43 @@ +//==- AnnexKDetection.cpp - Annex K availability detection -------*- C++ -*-==// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file contains the implementation of utilities for detecting C11 Annex K +// (Bounds-checking interfaces) availability. +// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/AnnexKDetection.h" + +#include "clang/Basic/LangOptions.h" +#include "clang/Lex/Preprocessor.h" + +namespace clang::analysis { + +[[nodiscard]] bool isAnnexKAvailable(Preprocessor *PP, const LangOptions &LO) { + if (!LO.C11) + return false; + + assert(PP && "No Preprocessor registered."); + + if (!PP->isMacroDefined("__STDC_LIB_EXT1__") || + !PP->isMacroDefined("__STDC_WANT_LIB_EXT1__")) + return false; + + const auto *MI = + PP->getMacroInfo(PP->getIdentifierInfo("__STDC_WANT_LIB_EXT1__")); + if (!MI || MI->tokens_empty()) + return false; + + const Token &T = MI->tokens().back(); + if (!T.isLiteral() || !T.getLiteralData()) + return false; + + return StringRef(T.getLiteralData(), T.getLength()) == "1"; +} + +} // namespace clang::analysis diff --git a/clang/lib/Analysis/CMakeLists.txt b/clang/lib/Analysis/CMakeLists.txt index c41f1fd77e5b7..65f160e965d47 100644 --- a/clang/lib/Analysis/CMakeLists.txt +++ b/clang/lib/Analysis/CMakeLists.txt @@ -5,6 +5,7 @@ set(LLVM_LINK_COMPONENTS add_clang_library(clangAnalysis AnalysisDeclContext.cpp + AnnexKDetection.cpp BodyFarm.cpp CalledOnceCheck.cpp CFG.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp index 5e75c1c4a3abd..1b94ccdbc4b5e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp @@ -10,16 +10,18 @@ // //===----------------------------------------------------------------------===// -#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/AST/StmtVisitor.h" #include "clang/Analysis/AnalysisDeclContext.h" +#include "clang/Analysis/AnnexKDetection.h" #include "clang/Basic/TargetInfo.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringSwitch.h" #include "llvm/Support/raw_ostream.h" +#include <optional> using namespace clang; using namespace ento; @@ -34,6 +36,9 @@ static bool isArc4RandomAvailable(const ASTContext &Ctx) { } namespace { + +enum class ReportPolicy { All, Actionable, C11Only }; + struct ChecksFilter { bool check_bcmp = false; bool check_bcopy = false; @@ -50,6 +55,8 @@ struct ChecksFilter { bool check_UncheckedReturn = false; bool check_decodeValueOfObjCType = false; + ReportPolicy ReportMode = ReportPolicy::C11Only; + CheckerNameRef checkName_bcmp; CheckerNameRef checkName_bcopy; CheckerNameRef checkName_bzero; @@ -73,14 +80,16 @@ class WalkAST : public StmtVisitor<WalkAST> { IdentifierInfo *II_setid[num_setids]; const bool CheckRand; + const ChecksFilter &filter; + const bool ShouldReportAnnexKRelated; public: - WalkAST(BugReporter &br, AnalysisDeclContext* ac, - const ChecksFilter &f) - : BR(br), AC(ac), II_setid(), - CheckRand(isArc4RandomAvailable(BR.getContext())), - filter(f) {} + WalkAST(BugReporter &br, AnalysisDeclContext *ac, const ChecksFilter &f, + bool shouldReportAnnexKRelated) + : BR(br), AC(ac), II_setid(), + CheckRand(isArc4RandomAvailable(BR.getContext())), filter(f), + ShouldReportAnnexKRelated(shouldReportAnnexKRelated) {} // Statement visitor methods. void VisitCallExpr(CallExpr *CE); @@ -751,10 +760,8 @@ void WalkAST::checkCall_strcat(const CallExpr *CE, const FunctionDecl *FD) { void WalkAST::checkDeprecatedOrUnsafeBufferHandling(const CallExpr *CE, const FunctionDecl *FD) { - if (!filter.check_DeprecatedOrUnsafeBufferHandling) - return; - - if (!BR.getContext().getLangOpts().C11) + if (!filter.check_DeprecatedOrUnsafeBufferHandling || + !ShouldReportAnnexKRelated) return; // Issue a warning. ArgIndex == -1: Deprecated but not unsafe (has size @@ -1072,13 +1079,40 @@ void WalkAST::checkUncheckedReturnValue(CallExpr *CE) { //===----------------------------------------------------------------------===// namespace { + +// Determine whether to report Annex K related checks based on the +// reporting policy. +[[nodiscard]] bool shouldReportAnnexKRelated(BugReporter &BR, + const ChecksFilter &Filter) { + const bool IsAnnexKAvailable = analysis::isAnnexKAvailable( + &BR.getPreprocessor(), BR.getContext().getLangOpts()); + const bool IsC11OrLaterStandard = BR.getContext().getLangOpts().C11; + + switch (Filter.ReportMode) { + case ReportPolicy::All: + return true; + case ReportPolicy::Actionable: + return IsAnnexKAvailable; + case ReportPolicy::C11Only: + return IsC11OrLaterStandard; + } + llvm_unreachable("Unknown ReportPolicy value"); +} + class SecuritySyntaxChecker : public Checker<check::ASTCodeBody> { public: ChecksFilter filter; + mutable std::optional<bool> CachedShouldReportAnnexKRelated; void checkASTCodeBody(const Decl *D, AnalysisManager& mgr, BugReporter &BR) const { - WalkAST walker(BR, mgr.getAnalysisDeclContext(D), filter); + // Compute ShouldReportAnnexKRelated once per translation unit. + if (!CachedShouldReportAnnexKRelated.has_value()) { + CachedShouldReportAnnexKRelated = shouldReportAnnexKRelated(BR, filter); + } + + WalkAST walker(BR, mgr.getAnalysisDeclContext(D), filter, + *CachedShouldReportAnnexKRelated); walker.Visit(D->getBody()); } }; @@ -1113,5 +1147,35 @@ REGISTER_CHECKER(rand) REGISTER_CHECKER(vfork) REGISTER_CHECKER(FloatLoopCounter) REGISTER_CHECKER(UncheckedReturn) -REGISTER_CHECKER(DeprecatedOrUnsafeBufferHandling) + +void ento::registerDeprecatedOrUnsafeBufferHandling(CheckerManager &Mgr) { + SecuritySyntaxChecker *Checker = Mgr.getChecker<SecuritySyntaxChecker>(); + Checker->filter.check_DeprecatedOrUnsafeBufferHandling = true; + Checker->filter.checkName_DeprecatedOrUnsafeBufferHandling = + Mgr.getCurrentCheckerName(); + + // Parse ReportMode option (defaults to C11Only for backward compatibility) + StringRef ReportModeStr = Mgr.getAnalyzerOptions().getCheckerStringOption( + Mgr.getCurrentCheckerName(), "ReportMode"); + Checker->filter.ReportMode = ReportPolicy::C11Only; + auto RequestedReportPolicy = + llvm::StringSwitch<std::optional<ReportPolicy>>(ReportModeStr) + .Case("all", ReportPolicy::All) + .Case("actionable", ReportPolicy::Actionable) + .Case("c11-only", ReportPolicy::C11Only) + .Default({}); + if (!RequestedReportPolicy) + Mgr.reportInvalidCheckerOptionValue( + Checker, "ReportMode", + "one of the following values: \"all\", \"actionable\" or \"c11-only\" " + "(the default)"); + else + Checker->filter.ReportMode = *RequestedReportPolicy; +} + +bool ento::shouldRegisterDeprecatedOrUnsafeBufferHandling( + const CheckerManager &) { + return true; +} + REGISTER_CHECKER(decodeValueOfObjCType) diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 7df5fab0843ac..209061a89e8a8 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -3305,6 +3305,17 @@ void LikelyFalsePositiveSuppressionBRVisitor::finalizeVisitor( } } + // The analyzer issues false positives in uninitialized_construct_buf_dispatch + // and __uninitialized_construct_buf from stl_tempbuf.h, triggered by + // std::stable_sort and std::inplace_merge. + if (const auto *FD = dyn_cast<FunctionDecl>(D)) { + StringRef Name = FD->getName(); + if (Name.contains("uninitialized_construct_buf")) { + BR.markInvalid(getTag(), nullptr); + return; + } + } + for (const LocationContext *LCtx = N->getLocationContext(); LCtx; LCtx = LCtx->getParent()) { const auto *MD = dyn_cast<CXXMethodDecl>(LCtx->getDecl()); @@ -3323,6 +3334,14 @@ void LikelyFalsePositiveSuppressionBRVisitor::finalizeVisitor( return; } + // The analyzer issues false positives in _Temporary_buffer used by + // std::stable_sort and std::inplace_merge due to complex buffer + // management logic. + if (CD->getName() == "_Temporary_buffer") { + BR.markInvalid(getTag(), nullptr); + return; + } + // The analyzer issues a false positive on // std::shared_ptr<int> p(new int(1)); p = nullptr; // because it does not reason properly about temporary destructors. diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h index fadc09f65d536..e048a6a892c48 100644 --- a/clang/test/Analysis/Inputs/system-header-simulator.h +++ b/clang/test/Analysis/Inputs/system-header-simulator.h @@ -82,6 +82,7 @@ char *strcpy(char *restrict, const char *restrict); char *strncpy(char *restrict dst, const char *restrict src, size_t n); char *strsep(char **restrict stringp, const char *restrict delim); void *memcpy(void *restrict dst, const void *restrict src, size_t n); +void *memmove(void *dst, const void *src, size_t n); void *memset(void *s, int c, size_t n); typedef unsigned long __darwin_pthread_key_t; diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c index 4e1f5336a9040..96b0c12821746 100644 --- a/clang/test/Analysis/analyzer-config.c +++ b/clang/test/Analysis/analyzer-config.c @@ -122,6 +122,7 @@ // CHECK-NEXT: region-store-small-struct-limit = 2 // CHECK-NEXT: report-in-main-source-file = false // CHECK-NEXT: security.cert.env.InvalidPtr:InvalidatingGetEnv = false +// CHECK-NEXT: security.insecureAPI.DeprecatedOrUnsafeBufferHandling:ReportMode = c11-only // CHECK-NEXT: serialize-stats = false // CHECK-NEXT: silence-checkers = "" // CHECK-NEXT: stable-report-filename = false diff --git a/clang/test/Analysis/security-deprecated-buffer-handling-report-modes.c b/clang/test/Analysis/security-deprecated-buffer-handling-report-modes.c new file mode 100644 index 0000000000000..c7e6e3d1a5ca2 --- /dev/null +++ b/clang/test/Analysis/security-deprecated-buffer-handling-report-modes.c @@ -0,0 +1,40 @@ +// DEFINE: %{analyze-cmd} = %clang_analyze_cc1 %s \ +// DEFINE: -analyzer-checker=security.insecureAPI.DeprecatedOrUnsafeBufferHandling + +// DEFINE: %{ReportMode} = -analyzer-config security.insecureAPI.DeprecatedOrUnsafeBufferHandling:ReportMode +// DEFINE: %{EnableAnnexK} = -D__STDC_LIB_EXT1__=200509L -D__STDC_WANT_LIB_EXT1__=1 + +// These cases should warn: +// RUN: %{analyze-cmd} -std=gnu99 %{ReportMode}=all -verify=common +// RUN: %{analyze-cmd} -std=gnu11 -verify=common +// RUN: %{analyze-cmd} -std=gnu11 %{ReportMode}=all -verify=common +// RUN: %{analyze-cmd} -std=gnu11 %{ReportMode}=c11-only -verify=common +// RUN: %{analyze-cmd} -std=gnu11 %{ReportMode}=actionable %{EnableAnnexK} -verify=common + +// These cases should not warn: +// RUN: %{analyze-cmd} -std=gnu99 -verify=no-warning +// RUN: %{analyze-cmd} -std=gnu99 %{ReportMode}=actionable -verify=no-warning +// RUN: %{analyze-cmd} -std=gnu99 %{ReportMode}=c11-only -verify=no-warning +// RUN: %{analyze-cmd} -std=gnu11 %{ReportMode}=actionable -verify=no-warning + +#include "Inputs/system-header-simulator.h" + +extern char buf[128]; +extern char src[128]; + +// no-warning-no-diagnostics + +void test_memcpy(void) { + memcpy(buf, src, 10); + // common-warning@-1{{Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard}} +} + +void test_memset(void) { + memset(buf, 0, 10); + // common-warning@-1{{Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard}} +} + +void test_memmove(void) { + memmove(buf, src, 10); + // common-warning@-1{{Call to function 'memmove' is insecure as it does not provide security checks introduced in the C11 standard}} +} `````````` </details> https://github.com/llvm/llvm-project/pull/177376 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
