From 8d9b4ba6e5369c5f772a6e6a419311f836677dee Mon Sep 17 00:00:00 2001 From: Tomasz Leman Date: Tue, 2 Jun 2026 17:45:45 +0200 Subject: [PATCH] Add check: invalidPointerOverlapTest for unsafe pointer overflow comparison Add a new inconclusive warning that detects the pointer overlap idiom p1 >= p2 && p1 < p2 + n (or its mirror / cast variants) where p1 and p2 are distinct pointers and n is an unsigned offset, e.g. the memcpy_s overlap check '(dest >= src && dest < src + count) || ...'. The 'p1 < p2 + n' clause assumes 'p2 + n' does not overflow past the end of the object. If n is large enough that 'p2 + n' wraps (UB), some mainstream compilers assume the wrap cannot happen and fold the comparison, silently dropping the check. The remedy in user code is to cast the pointers to uintptr_t and compare in integer space, with an explicit overflow guard. To avoid false positives, the warning only fires when the comparison is paired (via &&) with another comparison of the same two pointers, which identifies the overlap idiom; plain bounds checks like 'p < buf + len' and room checks like 'cur + need <= limit' are not flagged. The check is gated behind --enable=inconclusive. The same-pointer case 'p < p + n' is left to the existing invalidTestForOverflow check. This pattern was found in a real memcpy_s overlap check via fuzzing with UndefinedBehaviorSanitizer; the check is added so similar issues can be caught statically in the future. Signed-off-by: Tomasz Leman --- lib/checkcondition.cpp | 98 ++++++++++++++++++++++++++++++++++++++++++ lib/checkcondition.h | 1 + test/testcondition.cpp | 79 ++++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 72b61fd68b9..ecfe1e7c3e3 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1687,6 +1687,63 @@ void CheckConditionImpl::alwaysTrueFalseError(const Token* tok, const Token* con (alwaysTrue ? CWE571 : CWE570), Certainty::normal); } +// Strip enclosing casts so that `(char *)p` and `p` compare equal. +static const Token* skipPointerCast(const Token* tok) +{ + while (tok && tok->isCast() && tok->astOperand1()) + tok = tok->astOperand1(); + return tok; +} + +// Does `cmp` compare the same two pointers as p1/p2 (cast-stripped, either order)? +static bool comparesSamePointers(const Token* cmp, const Token* p1, const Token* p2, const Settings& settings) +{ + if (!cmp || !cmp->isComparisonOp() || !cmp->isBinaryOp()) + return false; + const Token* a = skipPointerCast(cmp->astOperand1()); + const Token* b = skipPointerCast(cmp->astOperand2()); + if (!a || !b) + return false; + return (isSameExpression(true, a, p1, settings, true, false) && + isSameExpression(true, b, p2, settings, true, false)) || + (isSameExpression(true, a, p2, settings, true, false) && + isSameExpression(true, b, p1, settings, true, false)); +} + +// Search a condition subtree for a comparison of the same two pointers p1/p2. +static bool hasPairedPointerCompare(const Token* tok, const Token* p1, const Token* p2, const Settings& settings) +{ + if (!tok) + return false; + if (comparesSamePointers(tok, p1, p2, settings)) + return true; + return hasPairedPointerCompare(tok->astOperand1(), p1, p2, settings) || + hasPairedPointerCompare(tok->astOperand2(), p1, p2, settings); +} + +// Recognize the pointer overlap idiom: p1 >= p2 && p1 < p2 + n (or mirror). +// `cmpTok` is the `p1 < p2 + n` comparison, `p1` the bare pointer operand and +// `p2` the pointer inside the `+`. We require a sibling comparison (under &&) of +// the *same two distinct pointers*; a plain bounds check `p < buf + len` has no +// such sibling and is therefore not flagged. +static bool isOverlapIdiom(const Token* cmpTok, const Token* p1, const Token* p2, const Settings& settings) +{ + const Token* p1base = skipPointerCast(p1); + const Token* p2base = skipPointerCast(p2); + if (!p1base || !p2base) + return false; + // distinct pointers only (the pastParent(); parent && parent->str() == "&&"; child = parent, parent = parent->astParent()) { + const Token* other = (parent->astOperand1() == child) ? parent->astOperand2() : parent->astOperand1(); + if (hasPairedPointerCompare(other, p1base, p2base, settings)) + return true; + } + return false; +} + void CheckConditionImpl::checkInvalidTestForOverflow() { // Interesting blogs: @@ -1771,10 +1828,50 @@ void CheckConditionImpl::checkInvalidTestForOverflow() continue; } } + + // Pointer overlap idiom: p1 >= p2 && p1 < p2 + n (or the mirror) + // Example (memcpy_s): (dest >= src && dest < src + count) || ... + // The `p1 < p2 + n` clause assumes `p2 + n` does not overflow past + // the end of the object. If `n` is large enough that `p2 + n` wraps + // (UB), compilers may fold the comparison and drop the check. + // We only warn when the comparison is paired (via &&) with another + // comparison of the *same two pointers*, which identifies the overlap + // idiom and avoids flagging plain bounds checks like `p < buf + len`. + if (isPointer && lhs->str() == "+" && mSettings.certainty.isEnabled(Certainty::inconclusive)) { + const Token *sibling = lhs->astSibling(); + if (sibling && sibling->valueType() && sibling->valueType()->pointer > 0) { + const Token *ptrOp = nullptr; + const Token *idxOp = nullptr; + for (const Token *op : exprTokens) { + if (!op || !op->valueType()) + continue; + if (op->valueType()->pointer > 0 && ptrOp == nullptr) + ptrOp = op; + else if (op->valueType()->pointer == 0 && + op->valueType()->isIntegral() && + op->valueType()->sign == ValueType::Sign::UNSIGNED && + idxOp == nullptr) + idxOp = op; + } + if (ptrOp && idxOp && isOverlapIdiom(tok, sibling, ptrOp, mSettings)) + invalidPointerOverlapTestError(tok); + } + } } } } +void CheckConditionImpl::invalidPointerOverlapTestError(const Token *tok) +{ + const std::string expr = (tok ? tok->expressionString() : std::string("p1 < p2 + n")); + const std::string errmsg = + "Invalid test for overflow '" + expr + "'; pointer overflow is undefined behavior. " + "The offset added to the pointer may be large enough to overflow, and some mainstream " + "compilers assume such overflow cannot happen and fold the comparison. Cast the pointers " + "to uintptr_t and compare in integer space instead."; + reportError(tok, Severity::warning, "invalidPointerOverlapTest", errmsg, uncheckedErrorConditionCWE, Certainty::inconclusive); +} + void CheckConditionImpl::invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace) { const std::string expr = (tok ? tok->expressionString() : std::string("x + c < x")); @@ -2135,6 +2232,7 @@ void CheckCondition::getErrorMessages(ErrorLogger& errorLogger, const Settings & c.clarifyConditionError(nullptr, true, false); c.alwaysTrueFalseError(nullptr, nullptr, nullptr); c.invalidTestForOverflow(nullptr, nullptr, "false"); + c.invalidPointerOverlapTestError(nullptr); c.pointerAdditionResultNotNullError(nullptr, nullptr); c.duplicateConditionalAssignError(nullptr, nullptr); c.assignmentInCondition(nullptr); diff --git a/lib/checkcondition.h b/lib/checkcondition.h index 232b2b5b340..20338265f02 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -174,6 +174,7 @@ class CPPCHECKLIB CheckConditionImpl : public CheckImpl { void alwaysTrueFalseError(const Token* tok, const Token* condition, const ValueFlow::Value* value); void invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace); + void invalidPointerOverlapTestError(const Token *tok); void pointerAdditionResultNotNullError(const Token *tok, const Token *calc); void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant = false); diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 3143b6eb201..de54f7721cb 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -6155,8 +6155,87 @@ class TestCondition : public TestFixture { check("int f(int x, int y) { return x - y > x; }"); ASSERT_EQUALS(MSG("x-y>x", "y<0"), errout_str()); + // x - y >= x check("int f(int x, int y) { return x - y >= x; }"); ASSERT_EQUALS(MSG("x-y>=x", "y<=0"), errout_str()); + + // Pointer overlap idiom: p1 >= p2 && p1 < p2 + n (distinct pointers, + // unsigned offset). Only flagged when paired (via &&) with a comparison + // of the same two pointers -- inconclusive. + const Settings sInc = settingsBuilder(settings0).certainty(Certainty::inconclusive).build(); + +#undef MSG +#define MSG(LOC, EXPR) "[test.cpp:2:" LOC "]: (warning, inconclusive) Invalid test for overflow '" EXPR "'; pointer overflow is undefined behavior. The offset added to the pointer may be large enough to overflow, and some mainstream compilers assume such overflow cannot happen and fold the comparison. Cast the pointers to uintptr_t and compare in integer space instead. [invalidPointerOverlapTest]\n" + + // the memcpy_s overlap check + check("int f(const char *dest, const char *src, size_t count) {\n" + " return dest >= src && dest < src + count;\n" + "}", sInc); + ASSERT_EQUALS(MSG("32", "dest= src;\n" + "}", sInc); + ASSERT_EQUALS(MSG("17", "dest= src && src + count > dest;\n" + "}", sInc); + ASSERT_EQUALS(MSG("39", "src+count>dest"), errout_str()); + + // casts on the offending clause (as in real memcpy_s) + check("int f(void *dest, const void *src, size_t count) {\n" + " return dest >= src && (char*)dest < (char*)src + count;\n" + "}", sInc); + ASSERT_EQUALS(MSG("39", "(char*)dest<(char*)src+count"), errout_str()); + + // the paired comparison may use any comparison operator + check("int f(const char *dest, const char *src, size_t count) {\n" + " return dest != src && dest < src + count;\n" + "}", sInc); + ASSERT_EQUALS(MSG("32", "dest NOT flagged + check("int f(const char *p, const char *buf, size_t len) {\n" + " return p < buf + len;\n" + "}", sInc); + ASSERT_EQUALS("", errout_str()); + + // room check -> NOT flagged + check("int f(const char *cur, const char *limit, size_t need) {\n" + " return cur + need <= limit;\n" + "}", sInc); + ASSERT_EQUALS("", errout_str()); + + // && partner compares unrelated operands -> NOT flagged + check("int f(const char *p, const char *buf, size_t len, int flag) {\n" + " return flag > 0 && p < buf + len;\n" + "}", sInc); + ASSERT_EQUALS("", errout_str()); + + // signed offset -> NOT flagged (overflow check is meaningful) + check("int f(const char *dest, const char *src, int count) {\n" + " return dest >= src && dest < src + count;\n" + "}", sInc); + ASSERT_EQUALS("", errout_str()); + + // not inconclusive -> NOT reported + check("int f(const char *dest, const char *src, size_t count) {\n" + " return dest >= src && dest < src + count;\n" + "}"); + ASSERT_EQUALS("", errout_str()); + + // same pointer var on both sides -> existing check applies, not the new one + check("int f(const char *p, size_t n) {\n" + " return p < p + n;\n" + "}", sInc); + ASSERT_EQUALS( + "[test.cpp:2:14]: (warning) Invalid test for overflow 'p