llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> Before this commit, there were two alpha checkers that used different algorithms/logic for detecting out of bounds memory access: the old `alpha.security.ArrayBound` and the experimental, more complex `alpha.security.ArrayBoundV2`. After lots of quality improvement commits ArrayBoundV2 is now stable enough to be moved out of the alpha stage. As indexing (and dereference) are common operations, it still produces a significant amount of false positives, but not much more than e.g. `core.NullDereference` or `core.UndefinedBinaryOperatorResult`, so it should be acceptable as a non-`core` checker. At this point `alpha.security.ArrayBound` became obsolete (there is a better tool for the same task), so I'm removing it from the codebase. With this I can eliminate the ugly "V2" version mark almost everywhere and rename `alpha.security.ArrayBoundV2` to `security.ArrayBound`. (The version mark is preserved in the filename "ArrayBoundCheckerV2", to ensure a clear git history. I'll rename it to "ArrayBoundChecker.cpp" in a separate commit.) This commit adapts the unit tests of `alpha.security.ArrayBound` to testing the new `security.ArrayBound` (= old ArrayBoundV2). Currently the names of the test files are very haphazard, I'll probably create a separate followup commit that consolidates this. ----- The effects of enabling this checker (compared to a run where only the other stable checkers are running): | Project | New Reports | Resolved Reports | |---------|-------------|------------------| | memcached | [2 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=memcached_1.6.8_edonnag_llvm-main_1883de3&newcheck=memcached_1.6.8_edonnag_llvm-main_f8aa6f1&diff-type=New) | [0 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=memcached_1.6.8_edonnag_llvm-main_1883de3&newcheck=memcached_1.6.8_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | tmux | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tmux_2.6_edonnag_llvm-main_1883de3&newcheck=tmux_2.6_edonnag_llvm-main_f8aa6f1&diff-type=New) | [0 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tmux_2.6_edonnag_llvm-main_1883de3&newcheck=tmux_2.6_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | curl | [5 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_edonnag_llvm-main_1883de3&newcheck=curl_curl-7_66_0_edonnag_llvm-main_f8aa6f1&diff-type=New) | [2 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_edonnag_llvm-main_1883de3&newcheck=curl_curl-7_66_0_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | twin | [15 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=twin_v0.8.1_edonnag_llvm-main_1883de3&newcheck=twin_v0.8.1_edonnag_llvm-main_f8aa6f1&diff-type=New) | [2 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=twin_v0.8.1_edonnag_llvm-main_1883de3&newcheck=twin_v0.8.1_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | vim | [57 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=vim_v8.2.1920_edonnag_llvm-main_1883de3&newcheck=vim_v8.2.1920_edonnag_llvm-main_f8aa6f1&diff-type=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=vim_v8.2.1920_edonnag_llvm-main_1883de3&newcheck=vim_v8.2.1920_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | openssl | [21 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_edonnag_llvm-main_1883de3&newcheck=openssl_openssl-3.0.0-alpha7_edonnag_llvm-main_f8aa6f1&diff-type=New) | [0 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_edonnag_llvm-main_1883de3&newcheck=openssl_openssl-3.0.0-alpha7_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | sqlite | [16 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_edonnag_llvm-main_1883de3&newcheck=sqlite_version-3.33.0_edonnag_llvm-main_f8aa6f1&diff-type=New) | [3 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_edonnag_llvm-main_1883de3&newcheck=sqlite_version-3.33.0_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | ffmpeg | [187 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_edonnag_llvm-main_1883de3&newcheck=ffmpeg_n4.3.1_edonnag_llvm-main_f8aa6f1&diff-type=New) | [27 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_edonnag_llvm-main_1883de3&newcheck=ffmpeg_n4.3.1_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | postgres | [66 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_edonnag_llvm-main_1883de3&newcheck=postgres_REL_13_0_edonnag_llvm-main_f8aa6f1&diff-type=New) | [6 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_edonnag_llvm-main_1883de3&newcheck=postgres_REL_13_0_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | tinyxml2 | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tinyxml2_8.0.0_edonnag_llvm-main_1883de3&newcheck=tinyxml2_8.0.0_edonnag_llvm-main_f8aa6f1&diff-type=New) | [0 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tinyxml2_8.0.0_edonnag_llvm-main_1883de3&newcheck=tinyxml2_8.0.0_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | libwebm | [23 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=libwebm_libwebm-1.0.0.27_edonnag_llvm-main_1883de3&newcheck=libwebm_libwebm-1.0.0.27_edonnag_llvm-main_f8aa6f1&diff-type=New) | [0 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=libwebm_libwebm-1.0.0.27_edonnag_llvm-main_1883de3&newcheck=libwebm_libwebm-1.0.0.27_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | xerces | [4 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=xerces_v3.2.3_edonnag_llvm-main_1883de3&newcheck=xerces_v3.2.3_edonnag_llvm-main_f8aa6f1&diff-type=New) | [2 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=xerces_v3.2.3_edonnag_llvm-main_1883de3&newcheck=xerces_v3.2.3_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | bitcoin | [9 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=bitcoin_v0.20.1_edonnag_llvm-main_1883de3&newcheck=bitcoin_v0.20.1_edonnag_llvm-main_f8aa6f1&diff-type=New) | [0 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=bitcoin_v0.20.1_edonnag_llvm-main_1883de3&newcheck=bitcoin_v0.20.1_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | protobuf | [10 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=protobuf_v3.13.0_edonnag_llvm-main_1883de3&newcheck=protobuf_v3.13.0_edonnag_llvm-main_f8aa6f1&diff-type=New) | [2 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=protobuf_v3.13.0_edonnag_llvm-main_1883de3&newcheck=protobuf_v3.13.0_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) | qtbase | [69 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=qtbase_v6.2.0_edonnag_llvm-main_1883de3&newcheck=qtbase_v6.2.0_edonnag_llvm-main_f8aa6f1&diff-type=New) | [5 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=qtbase_v6.2.0_edonnag_llvm-main_1883de3&newcheck=qtbase_v6.2.0_edonnag_llvm-main_f8aa6f1&diff-type=Resolved) --- Patch is 38.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/125534.diff 26 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (+5) - (modified) clang/docs/analyzer/checkers.rst (+60-73) - (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+35-32) - (removed) clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp (-92) - (modified) clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp (+28-22) - (modified) clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt (-1) - (modified) clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp (+4-2) - (modified) clang/lib/StaticAnalyzer/Core/MemRegion.cpp (+2-3) - (modified) clang/test/Analysis/index-type.c (+2-2) - (modified) clang/test/Analysis/misc-ps-region-store.m (+11-6) - (modified) clang/test/Analysis/no-outofbounds.c (+1-1) - (renamed) clang/test/Analysis/out-of-bounds-constraint-check.c (+1-1) - (modified) clang/test/Analysis/out-of-bounds-diagnostics.c (+1-1) - (modified) clang/test/Analysis/out-of-bounds-new.cpp (+1-1) - (modified) clang/test/Analysis/out-of-bounds-notes.c (+1-1) - (modified) clang/test/Analysis/out-of-bounds.c (+1-1) - (modified) clang/test/Analysis/outofbound-notwork.c (+4-4) - (modified) clang/test/Analysis/outofbound.c (+15-13) - (removed) clang/test/Analysis/rdar-6541136-region.c (-27) - (modified) clang/test/Analysis/runtime-regression.c (+1-1) - (modified) clang/test/Analysis/taint-diagnostic-visitor.c (+1-1) - (modified) clang/test/Analysis/taint-generic.c (+2-2) - (modified) clang/test/Analysis/taint-generic.cpp (+1-1) - (modified) clang/www/analyzer/open_projects.html (-13) - (modified) clang/www/analyzer/potential_checkers.html (+1-19) - (modified) llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn (-1) ``````````diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index a220e57d0b3222..a3aad565472eb4 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -231,6 +231,11 @@ Improvements Moved checkers ^^^^^^^^^^^^^^ +- After lots of improvements, the checker ``alpha.security.ArrayBoundV2`` is + renamed to ``security.ArrayBound``. As this checker is stable now, the old + checker ``alpha.security.ArrayBound`` (which was searching for the same kind + of bugs with an different, simpler and less accurate algorithm) is removed. + .. _release-notes-sanitizers: Sanitizers diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index e093b2d672a74e..707067358fdfe3 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -1332,10 +1332,69 @@ security Security related checkers. +.. _security-ArrayBound: + +security.ArrayBound (C, C++) +"""""""""""""""""""""""""""" +Report out of bounds access to memory that is before the start or after the end +of the accessed region (array, heap-allocated region, string literal etc.). +This usually means incorrect indexing, but the checker also detects access via +the operators ``*`` and ``->``. + +.. code-block:: c + + void test_underflow(int x) { + int buf[100][100]; + if (x < 0) + buf[0][x] = 1; // warn + } + + void test_overflow() { + int buf[100]; + int *p = buf + 100; + *p = 1; // warn + } + +If checkers like :ref:`unix-Malloc` or :ref:`cplusplus-NewDelete` are enabled +to model the behavior of ``malloc()``, ``operator new`` and similar +allocators), then this checker can also reports out of bounds access to +dynamically allocated memory: + +.. code-block:: cpp + + int *test_dynamic() { + int *mem = new int[100]; + mem[-1] = 42; // warn + return mem; + } + +In uncertain situations (when the checker can neither prove nor disprove that +overflow occurs), the checker assumes that the the index (more precisely, the +memory offeset) is within bounds. + +However, if :ref:`optin-taint-GenericTaint` is enabled and the index/offset is +tainted (i.e. it is influenced by an untrusted souce), then this checker +reports the potential out of bounds access: + +.. code-block:: c + + void test_with_tainted_index() { + char s[] = "abc"; + int x = getchar(); + char c = s[x]; // warn: potential out of bounds access with tainted index + } + +.. note:: + + This checker is an improved and renamed version of the checker that was + previously known as ``alpha.security.ArrayBoundV2``. The old checker + ``alpha.security.ArrayBound`` was removed when the (previously + "experimental") V2 variant became stable enough for regular use. + .. _security-cert-env-InvalidPtr: security.cert.env.InvalidPtr -"""""""""""""""""""""""""""""""""" +"""""""""""""""""""""""""""" Corresponds to SEI CERT Rules `ENV31-C <https://wiki.sei.cmu.edu/confluence/display/c/ENV31-C.+Do+not+rely+on+an+environment+pointer+following+an+operation+that+may+invalidate+it>`_ and `ENV34-C <https://wiki.sei.cmu.edu/confluence/display/c/ENV34-C.+Do+not+store+pointers+returned+by+certain+functions>`_. @@ -3216,78 +3275,6 @@ Warns against using one vs. many plural pattern in code when generating localize alpha.security ^^^^^^^^^^^^^^ -.. _alpha-security-ArrayBound: - -alpha.security.ArrayBound (C) -""""""""""""""""""""""""""""" -Warn about buffer overflows (older checker). - -.. code-block:: c - - void test() { - char *s = ""; - char c = s[1]; // warn - } - - struct seven_words { - int c[7]; - }; - - void test() { - struct seven_words a, *p; - p = &a; - p[0] = a; - p[1] = a; - p[2] = a; // warn - } - - // note: requires unix.Malloc or - // alpha.unix.MallocWithAnnotations checks enabled. - void test() { - int *p = malloc(12); - p[3] = 4; // warn - } - - void test() { - char a[2]; - int *b = (int*)a; - b[1] = 3; // warn - } - -.. _alpha-security-ArrayBoundV2: - -alpha.security.ArrayBoundV2 (C) -""""""""""""""""""""""""""""""" -Warn about buffer overflows (newer checker). - -.. code-block:: c - - void test() { - char *s = ""; - char c = s[1]; // warn - } - - void test() { - int buf[100]; - int *p = buf; - p = p + 99; - p[1] = 1; // warn - } - - // note: compiler has internal check for this. - // Use -Wno-array-bounds to suppress compiler warning. - void test() { - int buf[100][100]; - buf[0][-1] = 1; // warn - } - - // note: requires optin.taint check turned on. - void test() { - char s[] = "abc"; - int x = getchar(); - char c = s[x]; // warn: index is tainted - } - .. _alpha-security-ReturnPtrRange: alpha.security.ReturnPtrRange (C) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 1361da46c3c81d..9bf491eac1e0eb 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -989,30 +989,41 @@ def decodeValueOfObjCType : Checker<"decodeValueOfObjCType">, let ParentPackage = Security in { -def FloatLoopCounter : Checker<"FloatLoopCounter">, - HelpText<"Warn on using a floating point value as a loop counter (CERT: " - "FLP30-C, FLP30-CPP)">, - Dependencies<[SecuritySyntaxChecker]>, - Documentation<HasDocumentation>; - -def MmapWriteExecChecker : Checker<"MmapWriteExec">, - HelpText<"Warn on mmap() calls with both writable and executable access">, - Documentation<HasDocumentation>; - -def PointerSubChecker : Checker<"PointerSub">, - HelpText<"Check for pointer subtractions on two pointers pointing to " - "different memory chunks">, - Documentation<HasDocumentation>; - -def PutenvStackArray : Checker<"PutenvStackArray">, - HelpText<"Finds calls to the function 'putenv' which pass a pointer to " - "an automatic (stack-allocated) array as the argument.">, - Documentation<HasDocumentation>; - -def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">, - HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and " - "'setuid(getuid())' (CERT: POS36-C)">, - Documentation<HasDocumentation>; + def ArrayBoundChecker : Checker<"ArrayBound">, + HelpText<"Warn about out of bounds access to memory">, + Documentation<HasDocumentation>; + + def FloatLoopCounter + : Checker<"FloatLoopCounter">, + HelpText< + "Warn on using a floating point value as a loop counter (CERT: " + "FLP30-C, FLP30-CPP)">, + Dependencies<[SecuritySyntaxChecker]>, + Documentation<HasDocumentation>; + + def MmapWriteExecChecker + : Checker<"MmapWriteExec">, + HelpText< + "Warn on mmap() calls with both writable and executable access">, + Documentation<HasDocumentation>; + + def PointerSubChecker + : Checker<"PointerSub">, + HelpText<"Check for pointer subtractions on two pointers pointing to " + "different memory chunks">, + Documentation<HasDocumentation>; + + def PutenvStackArray + : Checker<"PutenvStackArray">, + HelpText<"Finds calls to the function 'putenv' which pass a pointer to " + "an automatic (stack-allocated) array as the argument.">, + Documentation<HasDocumentation>; + + def SetgidSetuidOrderChecker + : Checker<"SetgidSetuidOrder">, + HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and " + "'setuid(getuid())' (CERT: POS36-C)">, + Documentation<HasDocumentation>; } // end "security" @@ -1035,14 +1046,6 @@ let ParentPackage = ENV in { let ParentPackage = SecurityAlpha in { -def ArrayBoundChecker : Checker<"ArrayBound">, - HelpText<"Warn about buffer overflows (older checker)">, - Documentation<HasDocumentation>; - -def ArrayBoundCheckerV2 : Checker<"ArrayBoundV2">, - HelpText<"Warn about buffer overflows (newer checker)">, - Documentation<HasDocumentation>; - def ReturnPointerRangeChecker : Checker<"ReturnPtrRange">, HelpText<"Check for an out-of-bound pointer being returned to callers">, Documentation<HasDocumentation>; diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp deleted file mode 100644 index c990ad138f8905..00000000000000 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ /dev/null @@ -1,92 +0,0 @@ -//== ArrayBoundChecker.cpp ------------------------------*- 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 defines ArrayBoundChecker, which is a path-sensitive check -// which looks for an out-of-bound array element access. -// -//===----------------------------------------------------------------------===// - -#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" -#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" -#include "clang/StaticAnalyzer/Core/Checker.h" -#include "clang/StaticAnalyzer/Core/CheckerManager.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" - -using namespace clang; -using namespace ento; - -namespace { -class ArrayBoundChecker : - public Checker<check::Location> { - const BugType BT{this, "Out-of-bound array access"}; - -public: - void checkLocation(SVal l, bool isLoad, const Stmt* S, - CheckerContext &C) const; -}; -} - -void ArrayBoundChecker::checkLocation(SVal l, bool isLoad, const Stmt* LoadS, - CheckerContext &C) const { - // Check for out of bound array element access. - const MemRegion *R = l.getAsRegion(); - if (!R) - return; - - const ElementRegion *ER = dyn_cast<ElementRegion>(R); - if (!ER) - return; - - // Get the index of the accessed element. - DefinedOrUnknownSVal Idx = ER->getIndex().castAs<DefinedOrUnknownSVal>(); - - // Zero index is always in bound, this also passes ElementRegions created for - // pointer casts. - if (Idx.isZeroConstant()) - return; - - ProgramStateRef state = C.getState(); - - // Get the size of the array. - DefinedOrUnknownSVal ElementCount = getDynamicElementCount( - state, ER->getSuperRegion(), C.getSValBuilder(), ER->getValueType()); - - ProgramStateRef StInBound, StOutBound; - std::tie(StInBound, StOutBound) = state->assumeInBoundDual(Idx, ElementCount); - if (StOutBound && !StInBound) { - ExplodedNode *N = C.generateErrorNode(StOutBound); - if (!N) - return; - - // FIXME: It would be nice to eventually make this diagnostic more clear, - // e.g., by referencing the original declaration or by saying *why* this - // reference is outside the range. - - // Generate a report for this bug. - auto report = std::make_unique<PathSensitiveBugReport>( - BT, "Access out-of-bound array element (buffer overflow)", N); - - report->addRange(LoadS->getSourceRange()); - C.emitReport(std::move(report)); - return; - } - - // Array bound check succeeded. From this point forward the array bound - // should always succeed. - C.addTransition(StInBound); -} - -void ento::registerArrayBoundChecker(CheckerManager &mgr) { - mgr.registerChecker<ArrayBoundChecker>(); -} - -bool ento::shouldRegisterArrayBoundChecker(const CheckerManager &mgr) { - return true; -} diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp index 6422933c8828a9..6f8d6dbd573f40 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp @@ -6,11 +6,17 @@ // //===----------------------------------------------------------------------===// // -// This file defines ArrayBoundCheckerV2, which is a path-sensitive check -// which looks for an out-of-bound array element access. +// This file defines security.ArrayBound, which is a path-sensitive checker +// that looks for out of bounds access of memory regions. // //===----------------------------------------------------------------------===// +// NOTE: The name of this file ends with "V2" because previously +// "ArrayBoundChecker.cpp" contained the implementation of another (older and +// simpler) checker that was called `alpha.security.ArrayBound`. +// TODO: Rename this file to "ArrayBoundChecker.cpp" when it won't be confused +// with that older file. + #include "clang/AST/CharUnits.h" #include "clang/AST/ParentMapContext.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" @@ -124,9 +130,9 @@ struct Messages { // callbacks, we'd need to duplicate the logic that evaluates these // expressions.) The `MemberExpr` callback would work as `PreStmt` but it's // defined as `PostStmt` for the sake of consistency with the other callbacks. -class ArrayBoundCheckerV2 : public Checker<check::PostStmt<ArraySubscriptExpr>, - check::PostStmt<UnaryOperator>, - check::PostStmt<MemberExpr>> { +class ArrayBoundChecker : public Checker<check::PostStmt<ArraySubscriptExpr>, + check::PostStmt<UnaryOperator>, + check::PostStmt<MemberExpr>> { BugType BT{this, "Out-of-bound access"}; BugType TaintBT{this, "Out-of-bound access", categories::TaintedData}; @@ -547,7 +553,7 @@ bool StateUpdateReporter::providesInformationAboutInteresting( return false; } -void ArrayBoundCheckerV2::performCheck(const Expr *E, CheckerContext &C) const { +void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { const SVal Location = C.getSVal(E); // The header ctype.h (from e.g. glibc) implements the isXXXXX() macros as @@ -670,9 +676,9 @@ void ArrayBoundCheckerV2::performCheck(const Expr *E, CheckerContext &C) const { C.addTransition(State, SUR.createNoteTag(C)); } -void ArrayBoundCheckerV2::markPartsInteresting(PathSensitiveBugReport &BR, - ProgramStateRef ErrorState, - NonLoc Val, bool MarkTaint) { +void ArrayBoundChecker::markPartsInteresting(PathSensitiveBugReport &BR, + ProgramStateRef ErrorState, + NonLoc Val, bool MarkTaint) { if (SymbolRef Sym = Val.getAsSymbol()) { // If the offset is a symbolic value, iterate over its "parts" with // `SymExpr::symbols()` and mark each of them as interesting. @@ -693,10 +699,10 @@ void ArrayBoundCheckerV2::markPartsInteresting(PathSensitiveBugReport &BR, } } -void ArrayBoundCheckerV2::reportOOB(CheckerContext &C, - ProgramStateRef ErrorState, Messages Msgs, - NonLoc Offset, std::optional<NonLoc> Extent, - bool IsTaintBug /*=false*/) const { +void ArrayBoundChecker::reportOOB(CheckerContext &C, ProgramStateRef ErrorState, + Messages Msgs, NonLoc Offset, + std::optional<NonLoc> Extent, + bool IsTaintBug /*=false*/) const { ExplodedNode *ErrorNode = C.generateErrorNode(ErrorState); if (!ErrorNode) @@ -725,7 +731,7 @@ void ArrayBoundCheckerV2::reportOOB(CheckerContext &C, C.emitReport(std::move(BR)); } -bool ArrayBoundCheckerV2::isFromCtypeMacro(const Stmt *S, ASTContext &ACtx) { +bool ArrayBoundChecker::isFromCtypeMacro(const Stmt *S, ASTContext &ACtx) { SourceLocation Loc = S->getBeginLoc(); if (!Loc.isMacroID()) return false; @@ -744,7 +750,7 @@ bool ArrayBoundCheckerV2::isFromCtypeMacro(const Stmt *S, ASTContext &ACtx) { (MacroName == "isupper") || (MacroName == "isxdigit")); } -bool ArrayBoundCheckerV2::isInAddressOf(const Stmt *S, ASTContext &ACtx) { +bool ArrayBoundChecker::isInAddressOf(const Stmt *S, ASTContext &ACtx) { ParentMapContext &ParentCtx = ACtx.getParentMapContext(); do { const DynTypedNodeList Parents = ParentCtx.getParents(*S); @@ -756,10 +762,10 @@ bool ArrayBoundCheckerV2::isInAddressOf(const Stmt *S, ASTContext &ACtx) { return UnaryOp && UnaryOp->getOpcode() == UO_AddrOf; } -bool ArrayBoundCheckerV2::isIdiomaticPastTheEndPtr(const Expr *E, - ProgramStateRef State, - NonLoc Offset, NonLoc Limit, - CheckerContext &C) { +bool ArrayBoundChecker::isIdiomaticPastTheEndPtr(const Expr *E, + ProgramStateRef State, + NonLoc Offset, NonLoc Limit, + CheckerContext &C) { if (isa<ArraySubscriptExpr>(E) && isInAddressOf(E, C.getASTContext())) { auto [EqualsToThreshold, NotEqualToThreshold] = compareValueToThreshold( State, Offset, Limit, C.getSValBuilder(), /*CheckEquality=*/true); @@ -768,10 +774,10 @@ bool ArrayBoundCheckerV2::isIdiomaticPastTheEndPtr(const Expr *E, return false; } -void ento::registerArrayBoundCheckerV2(CheckerManager &mgr) { - mgr.registerChecker<ArrayBoundCheckerV2>(); +void ento::registerArrayBoundChecker(CheckerManager &mgr) { + mgr.registerChecker<ArrayBoundChecker>(); } -bool ento::shouldRegisterArrayBoundCheckerV2(const CheckerManager &mgr) { +bool ento::shouldRegisterArrayBoundChecker(const CheckerManager &mgr) { return true; } diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index fcbe8b864b6e41..ccff5d0ac3b964 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -7,7 +7,6 @@ set(LLVM_LINK_COMPONENTS add_clang_library(clangStaticAnalyzerCheckers AnalysisOrderChecker.cpp AnalyzerStatsChecker.cpp - ArrayBoundChecker.cpp ArrayBoundCheckerV2.cpp BasicObjCFoundationChecks.cpp BitwiseShiftChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 1a14f38e34f0e1..39dcaf02dbe258 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -547,8 +547,10 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, } return State; } - -// FIXME: This was originally copied from ArrayBoundChecker.cpp. Refactor? +// FIXME: The root of this logic was copied from the old checker +// alpha.security.ArrayBound (which is removed within this commit). +// It should be refactored to use the different, more sophisticated bounds +// checking logic used by the new checker ``security.ArrayBound``. ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C, ProgramStateRef state, AnyArgExpr Buffer, SVal Element, diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp index 2c5cd2cf7630f6..c81bb632b83e94 100644 --- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp +++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp @@ -891,9 +891,8 @@ DefinedOrUnknownSVal MemRegionManager::getStaticSize(const MemRegion *MR, return Size; } - // FIXME: The following are being used in 'SimpleSValBuilder' and in - // 'ArrayBoundChecker::checkLocation' because there is no symbol to - // represent the regions more appropriately. + // FIXME: The following are being used in 'SimpleSValBuilder' because there + // is no symbol to represent the regions more appropriately. case MemRegion::BlockDataRegionKind: case MemRegion::BlockCodeRegionKind: case MemRegion::FunctionCodeRegionKind: diff --git a/clang/test/Analysis/index-type.c b/clang/test/Analysis/index-type.c index 997d45c1e5abad..818806c4aff3b5 100644 --- a/clang/test/Analysis/index... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/125534 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
