[analyzer] Delete alpha.security.MallocOverflow
#103059
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...because it is too noisy to be useful right now, and its architecture is terrible, so it can't act a starting point of future development. The main problem with this checker is that it tries to do (or at least fake) path-sensitive analysis without actually using the established path-sensitive analysis engine. Instead of actually tracking the symbolic values and the known constraints on them, this checker blindly gropes the AST and uses heuristics like "this variable was seen in a comparison operator expression that is not a loop condition, so it's probably not too large" (which was improved in a separate commit to at least ignore comparison operators that appear after the actual `malloc()` call). This might have been acceptable in 2011 (when this checker was added), but since then we developed a significantly better standard approach for analysis and this old relic doesn't deserve to remain in the codebase. Needless to say, this primitive approach causes lots of false positives (and presumably false negatives as well), which ensures that this alpha checker won't be missed by the users. It would be good to eventually have a stable, path-sensitive checker that could succeed in the task where this hacky implementation fails, but the first step towards that goal is taking out this old garbage, which confuses the potential users or contributors. (I don't have plans for reimplementing the goals of this checker. It could happen eventually, but right now I have many plans that are higher priority than this.)
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) Changes...because it is too noisy to be useful right now, and its architecture is terrible, so it can't act a starting point of future development. The main problem with this checker is that it tries to do (or at least fake) path-sensitive analysis without actually using the established path-sensitive analysis engine. Instead of actually tracking the symbolic values and the known constraints on them, this checker blindly gropes the AST and uses heuristics like "this variable was seen in a comparison operator expression that is not a loop condition, so it's probably not too large" (which was improved in a separate commit to at least ignore comparison operators that appear after the actual This might have been acceptable in 2011 (when this checker was added), but since then we developed a significantly better standard approach for analysis and this old relic doesn't deserve to remain in the codebase. Needless to say, this primitive approach causes lots of false positives (and presumably false negatives as well), which ensures that this alpha checker won't be missed by the users. It would be good to eventually have a stable, path-sensitive checker that could succeed in the task where this hacky implementation fails, but the first step towards that goal is removing this old garbage, which just confuses the potential users or contributors. (I don't have plans for reimplementing the goals of this checker. It could happen eventually, but right now I have many goals that are higher priority than this.) Patch is 22.42 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/103059.diff 9 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 46b0b7b9c82376..0bfbc995579d41 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -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)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 38b55a0eb0a7b0..fb4114619ac3d3 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1039,10 +1039,6 @@ def ArrayBoundCheckerV2 : Checker<"ArrayBoundV2">,
HelpText<"Warn about buffer overflows (newer checker)">,
Documentation<HasDocumentation>;
-def MallocOverflowSecurityChecker : Checker<"MallocOverflow">,
- 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>;
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 682cfa01bec963..414282d58f779f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -63,7 +63,6 @@ add_clang_library(clangStaticAnalyzerCheckers
MacOSKeychainAPIChecker.cpp
MacOSXAPIChecker.cpp
MallocChecker.cpp
- MallocOverflowSecurityChecker.cpp
MallocSizeofChecker.cpp
MismatchedIteratorChecker.cpp
MmapWriteExecChecker.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
deleted file mode 100644
index 3c8b38973c6b8c..00000000000000
--- a/clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
+++ /dev/null
@@ -1,341 +0,0 @@
-// MallocOverflowSecurityChecker.cpp - Check for malloc overflows -*- 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 checker detects a common memory allocation security flaw.
-// Suppose 'unsigned int n' comes from an untrusted source. If the
-// code looks like 'malloc (n * 4)', and an attacker can make 'n' be
-// say MAX_UINT/4+2, then instead of allocating the correct 'n' 4-byte
-// elements, this will actually allocate only two because of overflow.
-// Then when the rest of the program attempts to store values past the
-// second element, these values will actually overwrite other items in
-// the heap, probably allowing the attacker to execute arbitrary code.
-//
-//===----------------------------------------------------------------------===//
-
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
-#include "clang/AST/EvaluatedExprVisitor.h"
-#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
-#include "clang/StaticAnalyzer/Core/Checker.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
-#include "llvm/ADT/APSInt.h"
-#include "llvm/ADT/SmallVector.h"
-#include <optional>
-#include <utility>
-
-using namespace clang;
-using namespace ento;
-using llvm::APSInt;
-
-namespace {
-struct MallocOverflowCheck {
- const CallExpr *call;
- const BinaryOperator *mulop;
- const Expr *variable;
- APSInt maxVal;
-
- MallocOverflowCheck(const CallExpr *call, const BinaryOperator *m,
- const Expr *v, APSInt val)
- : call(call), mulop(m), variable(v), maxVal(std::move(val)) {}
-};
-
-class MallocOverflowSecurityChecker : public Checker<check::ASTCodeBody> {
-public:
- void checkASTCodeBody(const Decl *D, AnalysisManager &mgr,
- BugReporter &BR) const;
-
- void CheckMallocArgument(
- SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
- const CallExpr *TheCall, ASTContext &Context) const;
-
- void OutputPossibleOverflows(
- SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
- const Decl *D, BugReporter &BR, AnalysisManager &mgr) const;
-
-};
-} // end anonymous namespace
-
-// Return true for computations which evaluate to zero: e.g., mult by 0.
-static inline bool EvaluatesToZero(APSInt &Val, BinaryOperatorKind op) {
- return (op == BO_Mul) && (Val == 0);
-}
-
-void MallocOverflowSecurityChecker::CheckMallocArgument(
- SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
- const CallExpr *TheCall, ASTContext &Context) const {
-
- /* Look for a linear combination with a single variable, and at least
- one multiplication.
- Reject anything that applies to the variable: an explicit cast,
- conditional expression, an operation that could reduce the range
- of the result, or anything too complicated :-). */
- const Expr *e = TheCall->getArg(0);
- const BinaryOperator * mulop = nullptr;
- APSInt maxVal;
-
- for (;;) {
- maxVal = 0;
- e = e->IgnoreParenImpCasts();
- if (const BinaryOperator *binop = dyn_cast<BinaryOperator>(e)) {
- BinaryOperatorKind opc = binop->getOpcode();
- // TODO: ignore multiplications by 1, reject if multiplied by 0.
- if (mulop == nullptr && opc == BO_Mul)
- mulop = binop;
- if (opc != BO_Mul && opc != BO_Add && opc != BO_Sub && opc != BO_Shl)
- return;
-
- const Expr *lhs = binop->getLHS();
- const Expr *rhs = binop->getRHS();
- if (rhs->isEvaluatable(Context)) {
- e = lhs;
- maxVal = rhs->EvaluateKnownConstInt(Context);
- if (EvaluatesToZero(maxVal, opc))
- return;
- } else if ((opc == BO_Add || opc == BO_Mul) &&
- lhs->isEvaluatable(Context)) {
- maxVal = lhs->EvaluateKnownConstInt(Context);
- if (EvaluatesToZero(maxVal, opc))
- return;
- e = rhs;
- } else
- return;
- } else if (isa<DeclRefExpr, MemberExpr>(e))
- break;
- else
- return;
- }
-
- if (mulop == nullptr)
- return;
-
- // We've found the right structure of malloc argument, now save
- // the data so when the body of the function is completely available
- // we can check for comparisons.
-
- PossibleMallocOverflows.push_back(
- MallocOverflowCheck(TheCall, mulop, e, maxVal));
-}
-
-namespace {
-// A worker class for OutputPossibleOverflows.
-class CheckOverflowOps :
- public EvaluatedExprVisitor<CheckOverflowOps> {
-public:
- typedef SmallVectorImpl<MallocOverflowCheck> theVecType;
-
-private:
- theVecType &toScanFor;
- ASTContext &Context;
-
- bool isIntZeroExpr(const Expr *E) const {
- if (!E->getType()->isIntegralOrEnumerationType())
- return false;
- Expr::EvalResult Result;
- if (E->EvaluateAsInt(Result, Context))
- return Result.Val.getInt() == 0;
- return false;
- }
-
- static const Decl *getDecl(const DeclRefExpr *DR) { return DR->getDecl(); }
- static const Decl *getDecl(const MemberExpr *ME) {
- return ME->getMemberDecl();
- }
-
- template <typename T1>
- void Erase(const T1 *DR,
- llvm::function_ref<bool(const MallocOverflowCheck &)> Pred) {
- auto P = [DR, Pred](const MallocOverflowCheck &Check) {
- if (const auto *CheckDR = dyn_cast<T1>(Check.variable))
- return getDecl(CheckDR) == getDecl(DR) && Pred(Check);
- return false;
- };
- llvm::erase_if(toScanFor, P);
- }
-
- void CheckExpr(const Expr *E_p) {
- const Expr *E = E_p->IgnoreParenImpCasts();
- const auto PrecedesMalloc = [E, this](const MallocOverflowCheck &c) {
- return Context.getSourceManager().isBeforeInTranslationUnit(
- E->getExprLoc(), c.call->getExprLoc());
- };
- if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
- Erase<DeclRefExpr>(DR, PrecedesMalloc);
- else if (const auto *ME = dyn_cast<MemberExpr>(E)) {
- Erase<MemberExpr>(ME, PrecedesMalloc);
- }
- }
-
- // Check if the argument to malloc is assigned a value
- // which cannot cause an overflow.
- // e.g., malloc (mul * x) and,
- // case 1: mul = <constant value>
- // case 2: mul = a/b, where b > x
- void CheckAssignmentExpr(BinaryOperator *AssignEx) {
- bool assignKnown = false;
- bool numeratorKnown = false, denomKnown = false;
- APSInt denomVal;
- denomVal = 0;
-
- // Erase if the multiplicand was assigned a constant value.
- const Expr *rhs = AssignEx->getRHS();
- if (rhs->isEvaluatable(Context))
- assignKnown = true;
-
- // Discard the report if the multiplicand was assigned a value,
- // that can never overflow after multiplication. e.g., the assignment
- // is a division operator and the denominator is > other multiplicand.
- const Expr *rhse = rhs->IgnoreParenImpCasts();
- if (const BinaryOperator *BOp = dyn_cast<BinaryOperator>(rhse)) {
- if (BOp->getOpcode() == BO_Div) {
- const Expr *denom = BOp->getRHS()->IgnoreParenImpCasts();
- Expr::EvalResult Result;
- if (denom->EvaluateAsInt(Result, Context)) {
- denomVal = Result.Val.getInt();
- denomKnown = true;
- }
- const Expr *numerator = BOp->getLHS()->IgnoreParenImpCasts();
- if (numerator->isEvaluatable(Context))
- numeratorKnown = true;
- }
- }
- if (!assignKnown && !denomKnown)
- return;
- auto denomExtVal = denomVal.getExtValue();
-
- // Ignore negative denominator.
- if (denomExtVal < 0)
- return;
-
- const Expr *lhs = AssignEx->getLHS();
- const Expr *E = lhs->IgnoreParenImpCasts();
-
- auto pred = [assignKnown, numeratorKnown,
- denomExtVal](const MallocOverflowCheck &Check) {
- return assignKnown ||
- (numeratorKnown && (denomExtVal >= Check.maxVal.getExtValue()));
- };
-
- if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
- Erase<DeclRefExpr>(DR, pred);
- else if (const auto *ME = dyn_cast<MemberExpr>(E))
- Erase<MemberExpr>(ME, pred);
- }
-
- public:
- void VisitBinaryOperator(BinaryOperator *E) {
- if (E->isComparisonOp()) {
- const Expr * lhs = E->getLHS();
- const Expr * rhs = E->getRHS();
- // Ignore comparisons against zero, since they generally don't
- // protect against an overflow.
- if (!isIntZeroExpr(lhs) && !isIntZeroExpr(rhs)) {
- CheckExpr(lhs);
- CheckExpr(rhs);
- }
- }
- if (E->isAssignmentOp())
- CheckAssignmentExpr(E);
- EvaluatedExprVisitor<CheckOverflowOps>::VisitBinaryOperator(E);
- }
-
- /* We specifically ignore loop conditions, because they're typically
- not error checks. */
- void VisitWhileStmt(WhileStmt *S) {
- return this->Visit(S->getBody());
- }
- void VisitForStmt(ForStmt *S) {
- return this->Visit(S->getBody());
- }
- void VisitDoStmt(DoStmt *S) {
- return this->Visit(S->getBody());
- }
-
- CheckOverflowOps(theVecType &v, ASTContext &ctx)
- : EvaluatedExprVisitor<CheckOverflowOps>(ctx),
- toScanFor(v), Context(ctx)
- { }
- };
-}
-
-// OutputPossibleOverflows - We've found a possible overflow earlier,
-// now check whether Body might contain a comparison which might be
-// preventing the overflow.
-// This doesn't do flow analysis, range analysis, or points-to analysis; it's
-// just a dumb "is there a comparison" scan. The aim here is to
-// detect the most blatent cases of overflow and educate the
-// programmer.
-void MallocOverflowSecurityChecker::OutputPossibleOverflows(
- SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
- const Decl *D, BugReporter &BR, AnalysisManager &mgr) const {
- // By far the most common case: nothing to check.
- if (PossibleMallocOverflows.empty())
- return;
-
- // Delete any possible overflows which have a comparison.
- CheckOverflowOps c(PossibleMallocOverflows, BR.getContext());
- c.Visit(mgr.getAnalysisDeclContext(D)->getBody());
-
- // Output warnings for all overflows that are left.
- for (const MallocOverflowCheck &Check : PossibleMallocOverflows) {
- BR.EmitBasicReport(
- D, this, "malloc() size overflow", categories::UnixAPI,
- "the computation of the size of the memory allocation may overflow",
- PathDiagnosticLocation::createOperatorLoc(Check.mulop,
- BR.getSourceManager()),
- Check.mulop->getSourceRange());
- }
-}
-
-void MallocOverflowSecurityChecker::checkASTCodeBody(const Decl *D,
- AnalysisManager &mgr,
- BugReporter &BR) const {
-
- CFG *cfg = mgr.getCFG(D);
- if (!cfg)
- return;
-
- // A list of variables referenced in possibly overflowing malloc operands.
- SmallVector<MallocOverflowCheck, 2> PossibleMallocOverflows;
-
- for (CFG::iterator it = cfg->begin(), ei = cfg->end(); it != ei; ++it) {
- CFGBlock *block = *it;
- for (CFGBlock::iterator bi = block->begin(), be = block->end();
- bi != be; ++bi) {
- if (std::optional<CFGStmt> CS = bi->getAs<CFGStmt>()) {
- if (const CallExpr *TheCall = dyn_cast<CallExpr>(CS->getStmt())) {
- // Get the callee.
- const FunctionDecl *FD = TheCall->getDirectCallee();
-
- if (!FD)
- continue;
-
- // Get the name of the callee. If it's a builtin, strip off the
- // prefix.
- IdentifierInfo *FnInfo = FD->getIdentifier();
- if (!FnInfo)
- continue;
-
- if (FnInfo->isStr("malloc") || FnInfo->isStr("_MALLOC")) {
- if (TheCall->getNumArgs() == 1)
- CheckMallocArgument(PossibleMallocOverflows, TheCall,
- mgr.getASTContext());
- }
- }
- }
- }
- }
-
- OutputPossibleOverflows(PossibleMallocOverflows, D, BR, mgr);
-}
-
-void ento::registerMallocOverflowSecurityChecker(CheckerManager &mgr) {
- mgr.registerChecker<MallocOverflowSecurityChecker>();
-}
-
-bool ento::shouldRegisterMallocOverflowSecurityChecker(const CheckerManager &mgr) {
- return true;
-}
diff --git a/clang/test/Analysis/malloc-overflow.c b/clang/test/Analysis/malloc-overflow.c
deleted file mode 100644
index 03fe15bccb62ee..00000000000000
--- a/clang/test/Analysis/malloc-overflow.c
+++ /dev/null
@@ -1,150 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.MallocOverflow -verify %s
-
-#define NULL ((void *) 0)
-typedef __typeof__(sizeof(int)) size_t;
-extern void * malloc(size_t);
-
-void * f1(int n)
-{
- return malloc(n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
-}
-
-void * f2(int n)
-{
- return malloc(sizeof(int) * n); // // expected-warning {{the computation of the size of the memory allocation may overflow}}
-}
-
-void * f3(void)
-{
- return malloc(4 * sizeof(int)); // no-warning
-}
-
-struct s4
-{
- int n;
-};
-
-void * f4(struct s4 *s)
-{
- return malloc(s->n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
-}
-
-void * f5(struct s4 *s)
-{
- struct s4 s2 = *s;
- return malloc(s2.n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
-}
-
-void * f6(int n)
-{
- return malloc((n + 1) * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
-}
-
-extern void * malloc (size_t);
-
-void * f7(int n)
-{
- if (n > 10)
- return NULL;
- return malloc(n * sizeof(int)); // no-warning
-}
-
-void * f8(int n)
-{
- if (n < 10)
- return malloc(n * sizeof(int)); // no-warning
- else
- return NULL;
-}
-
-void * f9(int n)
-{
- int * x = malloc(n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
- for (int i = 0; i < n; i++)
- x[i] = i;
- return x;
-}
-
-void * f10(int n)
-{
- int * x = malloc(n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
- int i = 0;
- while (i < n)
- x[i++] = 0;
- return x;
-}
-
-void * f11(int n)
-{
- int * x = malloc(n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
- int i = 0;
- do {
- x[i++] = 0;
- } while (i < n);
- return x;
-}
-
-void * f12(int n)
-{
- n = (n > 10 ? 10 : n);
- int * x = malloc(n * sizeof(int)); // no-warning
- for (int i = 0; i < n; i++)
- x[i] = i;
- return x;
-}
-
-struct s13
-{
- int n;
-};
-
-void * f13(struct s13 *s)
-{
- if (s->n > 10)
- return NULL;
- return malloc(s->n * sizeof(int)); // no-warning
-}
-
-void * f14(int n)
-{
- if (n < 0)
- return NULL;
- return malloc(n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
-}
-
-void *check_before_malloc(int n, int x) {
- int *p = NULL;
- if (n > 10)
- return NULL;
- if (x == 42)
- p = malloc(n * sizeof(int)); // no-warning, the check precedes the allocation
-
- // Do some other stuff, e.g. initialize the memory.
- return p;
-}
-
-void *check_after_malloc(int n, int x) {
- int *p = NULL;
- if (x == 42)
- p = malloc(n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
-
- // The check is after the allocation!
- if (n > 10) {
- // Do something conditionally.
- }
- return p;
-}
-
-#define GREATER_THAN(lhs, rhs) (lhs > rhs)
-void *check_after_malloc_using_macros(int n, int x) {
- int *p = NULL;
- if (x == 42)
- p = malloc(n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
-
- if (GREATER_THAN(n, 10))
- return NULL;
-
- // Do some other stuff, e.g. initialize the memory.
- return p;
-}
-#undef GREATER_THAN
diff --git a/clang/test/Analysis/malloc-overflow.cpp b/clang/test/Analysis/malloc-overflow.cpp
deleted file mode 100644
index e070217cf7d8e2..00000000000000
--- a/clang/test/Analysis/malloc-overflow.cpp
+++ /dev/null
@@ -1,12 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.MallocOverflow -verify %s
-// expected-no-diagnostics
-
-class A {
-public:
- A& operator<<(const A &a);
-};
-
-void f() {
- A a = A(), b = A();
- a << b;
-}
diff --git a/clang/test/Analysis/malloc-overflow2.c b/clang/test/Analysis/malloc-overflow2.c
deleted file mode 100644
index 7c580602e682ab..00000000000000
--- a/clang/test/Analysis/malloc-overflow2.c
+++ /dev/null
@@ -1,40 +0,0 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-unkno...
[truncated]
|
|
I agree with Donat's proposal to remove this check, especially if the taint checker can be used to detect a "tainted" argument to an equivalent malloc() function. We see many false positives with the approach used in this checker to the extent that devs disable this checker. LGTM, but someone else must approve. Thanks |
steakhal
approved these changes
Aug 13, 2024
haoNoQ
approved these changes
Aug 13, 2024
| @@ -1,40 +0,0 @@ | |||
| // RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -analyzer-checker=alpha.security.MallocOverflow,unix -verify %s | |||
| // RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -analyzer-checker=alpha.security.MallocOverflow,unix,optin.portability -DPORTABILITY -verify %s | |||
There was a problem hiding this comment.
This test was enabling a few other checkers (including a few path-sensitive checkers), so it could have acted as a "true negatives" test for those checkers, so I think it may be slightly useful if we preserve it. Quite unlikely though.
There was a problem hiding this comment.
I agree that this might have some minor usefulness by accident, but I still think that it's better to remove it. (After all, we are not copying random "no results found" source files into our test directory...)
| HelpText<"Warn about buffer overflows (newer checker)">, | ||
| Documentation<HasDocumentation>; | ||
|
|
||
| def MallocOverflowSecurityChecker : Checker<"MallocOverflow">, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- this checker is so unreliable that it's extremely unlikely that somebody was relying on it;
- 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.
Yeah this should either be a "taint analysis" thing. Or a coding-convention thing that only works when the users are provided with a clear alternative, such as "please always use overflow-checked builtins when computing the size for malloc". And even then it might be better as a path-sensitive check, both for false positives and for false negatives, depending on how hard you want to go with that convention. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/2228 Here is the relevant piece of the build log for the reference: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
...because it is too noisy to be useful right now, and its architecture is terrible, so it can't act a starting point of future development.
The main problem with this checker is that it tries to do (or at least fake) path-sensitive analysis without actually using the established path-sensitive analysis engine.
Instead of actually tracking the symbolic values and the known constraints on them, this checker blindly gropes the AST and uses heuristics like "this variable was seen in a comparison operator expression that is not a loop condition, so it's probably not too large" (which was improved in a separate commit to at least ignore comparison operators that appear after the actual
malloc()call).This might have been acceptable in 2011 (when this checker was added), but since then we developed a significantly better standard approach for analysis and this old relic doesn't deserve to remain in the codebase.
Needless to say, this primitive approach causes lots of false positives (and presumably false negatives as well), which ensures that this alpha checker won't be missed by the users.
Moreover, the goals of this checker would be questionable even if it had a perfect implementation. It's very aggressive to assume that the argument of malloc can overflow by default (unless the checker sees a bounds check); and this produces too many false positives -- perhaps even for an optin checker. It may be possible to eventually create a useful (and properly path-sensitive) optin checker for these kinds of suspicious code, but this is a very low priority goal.
Also note that we already have
alpha.security.TaintedAllocwhich provides more practical heuristics for detecting somewhat similar "argument of malloc may be too large" vulnerabilities.