[clang] [llvm] [analyzer] Consolidate array bound checkers (PR #125534)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 3 09:15:43 PST 2025
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
More information about the cfe-commits
mailing list