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 <tomasz.m.leman@intel.com>

Author: tmleman

lib/checkcondition.cpp

@@ -1688,6 +1688,63 @@ void CheckCondition::alwaysTrueFalseError(const Token* tok, const Token* conditi
                 (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 p<p+n case is handled by invalidTestForOverflow)
+    if (isSameExpression(true, p1base, p2base, settings, true, false))
+        return false;
+    const Token* child = cmpTok;
+    for (const Token* parent = cmpTok->astParent(); 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 CheckCondition::checkInvalidTestForOverflow()
 {
     // Interesting blogs:
@@ -1772,10 +1829,50 @@ void CheckCondition::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 CheckCondition::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 CheckCondition::invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace)
 {
     const std::string expr = (tok ? tok->expressionString() : std::string("x + c < x"));
@@ -2136,6 +2233,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);

lib/checkcondition.h

@@ -149,6 +149,7 @@ class CPPCHECKLIB CheckCondition : public Check {
     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);

test/testcondition.cpp

@@ -6154,8 +6154,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+count"), errout_str());
+
+        // mirror order of the two clauses
+        check("int f(const char *dest, const char *src, size_t count) {\n"
+              "    return dest < src + count && dest >= src;\n"
+              "}", sInc);
+        ASSERT_EQUALS(MSG("17", "dest<src+count"), errout_str());
+
+        // offset on the left side of the comparison
+        check("int f(const char *dest, const char *src, size_t count) {\n"
+              "    return 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<src+count"), errout_str());
+
+        // --- negatives ---
+
+        // plain bounds check, no paired pointer comparison -> 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<p+n'; pointer overflow is undefined behavior. Some mainstream compilers remove such overflow tests when optimising the code and assume it's always true. [invalidTestForOverflow]\n",
+            errout_str());
     }
 
     void checkConditionIsAlwaysTrueOrFalseInsideIfWhile() {