Skip to content

[analyzer] Consolidate array bound checkers #125534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
133 changes: 60 additions & 73 deletions clang/docs/analyzer/checkers.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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>`_.

Expand Down Expand Up @@ -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)
Expand Down
67 changes: 35 additions & 32 deletions clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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>;
Expand Down
92 changes: 0 additions & 92 deletions clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp

This file was deleted.

Loading