Skip to content
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 @@ -347,6 +347,11 @@ Improvements
Moved checkers
^^^^^^^^^^^^^^

- The checker ``alpha.security.MallocOverflow`` was deleted because it was
badly implemented and its agressive logic produced too many false positives.
To detect too large arguments passed to malloc, consider using the checker
``alpha.taint.TaintedAlloc``.

.. _release-notes-sanitizers:

Sanitizers
Expand Down
43 changes: 0 additions & 43 deletions clang/docs/analyzer/checkers.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2951,49 +2951,6 @@ Warn about buffer overflows (newer checker).
char c = s[x]; // warn: index is tainted
}

.. _alpha-security-MallocOverflow:

alpha.security.MallocOverflow (C)
"""""""""""""""""""""""""""""""""
Check for overflows in the arguments to ``malloc()``.
It tries to catch ``malloc(n * c)`` patterns, where:

- ``n``: a variable or member access of an object
- ``c``: a constant foldable integral

This checker was designed for code audits, so expect false-positive reports.
One is supposed to silence this checker by ensuring proper bounds checking on
the variable in question using e.g. an ``assert()`` or a branch.

.. code-block:: c

void test(int n) {
void *p = malloc(n * sizeof(int)); // warn
}

void test2(int n) {
if (n > 100) // gives an upper-bound
return;
void *p = malloc(n * sizeof(int)); // no warning
}

void test3(int n) {
assert(n <= 100 && "Contract violated.");
void *p = malloc(n * sizeof(int)); // no warning
}

Limitations:

- The checker won't warn for variables involved in explicit casts,
since that might limit the variable's domain.
E.g.: ``(unsigned char)int x`` would limit the domain to ``[0,255]``.
The checker will miss the true-positive cases when the explicit cast would
not tighten the domain to prevent the overflow in the subsequent
multiplication operation.

- It is an AST-based checker, thus it does not make use of the
path-sensitive taint-analysis.

.. _alpha-security-MmapWriteExec:

alpha.security.MmapWriteExec (C)
Expand Down
4 changes: 0 additions & 4 deletions clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
Original file line number Diff line number Diff line change
Expand Up @@ -1039,10 +1039,6 @@ def ArrayBoundCheckerV2 : Checker<"ArrayBoundV2">,
HelpText<"Warn about buffer overflows (newer checker)">,
Documentation<HasDocumentation>;

def MallocOverflowSecurityChecker : Checker<"MallocOverflow">,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could keep this for a while to avoid breaking people's configurations, but given that the checker was alpha we're probably not strictly obliged to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to write a "this checker no longer exists, but references to its name should not be an error" placeholder in this TD file?

If you readily know a solution, I'd be happy to add it, but I don't want to spend time on researching it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's necessity. They could fork it and revert this change if they really wanted. I doubt that would be the case.
@NagyDonat If you really want to make this a backward-compatible change, you could add a new bool checker option like "enable-despite-this-checker-is-deprecated-and-will-be-removed", and leave the checker - until we branch off for clang-20 in mid January. But personally, I wouldn't want to wait for 4 months for removing an alpha checker.
We could reconsider this and clarify the policies if users complain that alpha checkers were removed without any notice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, we already have lots of precedent for moving and renaming alpha checkers without prior warning, so the "updated to the new version and the old command line invocation doesn't work without changes" situation is not a problem.

The only difference is that after this change they won't be able to restore the behavior by using a new name for the checker, but I think that this is acceptable, because:

  1. this checker is so unreliable that it's extremely unlikely that somebody was relying on it;
  2. deleting a checker is a conservative change: it removes the results of that particular checker, but it doesn't affects the results of the other checkers and doesn't hinder the investigation of the results by introducing lots of spammy results.

Based on this, I'm merging this commit now.

HelpText<"Check for overflows in the arguments to malloc()">,
Documentation<HasDocumentation>;

def MmapWriteExecChecker : Checker<"MmapWriteExec">,
HelpText<"Warn on mmap() calls that are both writable and executable">,
Documentation<HasDocumentation>;
Expand Down
1 change: 0 additions & 1 deletion clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ add_clang_library(clangStaticAnalyzerCheckers
MacOSKeychainAPIChecker.cpp
MacOSXAPIChecker.cpp
MallocChecker.cpp
MallocOverflowSecurityChecker.cpp
MallocSizeofChecker.cpp
MismatchedIteratorChecker.cpp
MmapWriteExecChecker.cpp
Expand Down
Loading
Loading