Fix #13876 (Document cstyleCast) [ci skip] (danmar#7539)

Author: danmar

lib/checkother.cpp

@@ -290,6 +290,44 @@ void CheckOther::suspiciousSemicolonError(const Token* tok)
                 "Suspicious use of ; at the end of '" + (tok ? tok->str() : std::string()) + "' statement.", CWE398, Certainty::normal);
 }
 
+/** @brief would it make sense to use dynamic_cast instead of C style cast? */
+static bool isDangerousTypeConversion(const Token* const tok)
+{
+    const Token* from = tok->astOperand1();
+    if (!from)
+        return false;
+    if (!tok->valueType() || !from->valueType())
+        return false;
+    if (tok->valueType()->typeScope != nullptr &&
+        tok->valueType()->typeScope == from->valueType()->typeScope)
+        return false;
+    if (tok->valueType()->type == from->valueType()->type &&
+        tok->valueType()->isPrimitive())
+        return false;
+    // cast from derived object to base object is safe..
+    if (tok->valueType()->typeScope && from->valueType()->typeScope) {
+        const Type* fromType = from->valueType()->typeScope->definedType;
+        const Type* toType = tok->valueType()->typeScope->definedType;
+        if (fromType && toType && fromType->isDerivedFrom(toType->name()))
+            return false;
+    }
+    const bool refcast = (tok->valueType()->reference != Reference::None);
+    if (!refcast && tok->valueType()->pointer == 0)
+        return false;
+    if (!refcast && from->valueType()->pointer == 0)
+        return false;
+
+    if (tok->valueType()->type == ValueType::Type::VOID || from->valueType()->type == ValueType::Type::VOID)
+        return false;
+    if (tok->valueType()->pointer == 0 && tok->valueType()->isIntegral())
+        // ok: (uintptr_t)ptr;
+        return false;
+    if (from->valueType()->pointer == 0 && from->valueType()->isIntegral())
+        // ok: (int *)addr;
+        return false;
+
+    return true;
+}
 
 //---------------------------------------------------------------------------
 // For C++ code, warn if C-style casts are used on pointer types
@@ -314,8 +352,11 @@ void CheckOther::warningOldStylePointerCast()
             tok = scope->bodyStart;
         for (; tok && tok != scope->bodyEnd; tok = tok->next()) {
             // Old style pointer casting..
-            if (tok->str() != "(")
+            if (!tok->isCast() || tok->isBinaryOp())
+                continue;
+            if (isDangerousTypeConversion(tok))
                 continue;
+            const Token* const errtok = tok;
             const Token* castTok = tok->next();
             while (Token::Match(castTok, "const|volatile|class|struct|union|%type%|::")) {
                 castTok = castTok->next();
@@ -332,7 +373,7 @@ void CheckOther::warningOldStylePointerCast()
                     isRef = true;
                 castTok = castTok->next();
             }
-            if ((!isPtr && !isRef) || !Token::Match(castTok, ") (| %name%|%num%|%bool%|%char%|%str%|&"))
+            if ((!isPtr && !isRef) || !Token::Match(castTok, ") (| %name%|%bool%|%char%|%str%|&"))
                 continue;
 
             if (Token::Match(tok->previous(), "%type%"))
@@ -351,7 +392,7 @@ void CheckOther::warningOldStylePointerCast()
                 continue;
 
             if (typeTok->tokType() == Token::eType || typeTok->tokType() == Token::eName)
-                cstyleCastError(tok, isPtr);
+                cstyleCastError(errtok, isPtr);
         }
     }
 }
@@ -367,6 +408,79 @@ void CheckOther::cstyleCastError(const Token *tok, bool isPtr)
                 "which kind of cast is expected.", CWE398, Certainty::normal);
 }
 
+void CheckOther::warningDangerousTypeCast()
+{
+    // Only valid on C++ code
+    if (!mTokenizer->isCPP())
+        return;
+    if (!mSettings->severity.isEnabled(Severity::warning) && !mSettings->isPremiumEnabled("cstyleCast"))
+        return;
+
+    logChecker("CheckOther::warningDangerousTypeCast"); // warning,c++
+
+    const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
+    for (const Scope * scope : symbolDatabase->functionScopes) {
+        const Token* tok;
+        if (scope->function && scope->function->isConstructor())
+            tok = scope->classDef;
+        else
+            tok = scope->bodyStart;
+        for (; tok && tok != scope->bodyEnd; tok = tok->next()) {
+            // Old style pointer casting..
+            if (!tok->isCast() || tok->isBinaryOp())
+                continue;
+
+            if (isDangerousTypeConversion(tok))
+                dangerousTypeCastError(tok, tok->valueType()->pointer > 0);
+        }
+    }
+}
+
+void CheckOther::dangerousTypeCastError(const Token *tok, bool isPtr)
+{
+    //const std::string type = isPtr ? "pointer" : "reference";
+    (void)isPtr;
+    reportError(tok, Severity::warning, "dangerousTypeCast",
+                "Potentially invalid type conversion in old-style C cast, clarify/fix with C++ cast",
+                CWE398, Certainty::normal);
+}
+
+void CheckOther::warningIntToPointerCast()
+{
+    if (!mSettings->severity.isEnabled(Severity::portability) && !mSettings->isPremiumEnabled("cstyleCast"))
+        return;
+
+    logChecker("CheckOther::warningIntToPointerCast"); // portability
+
+    for (const Token* tok = mTokenizer->tokens(); tok; tok = tok->next()) {
+        // pointer casting..
+        if (!tok->isCast())
+            continue;
+        const Token* from = tok->astOperand2() ? tok->astOperand2() : tok->astOperand1();
+        if (!from || !from->isNumber())
+            continue;
+        if (!tok->valueType() || tok->valueType()->pointer == 0)
+            continue;
+        if (!MathLib::isIntHex(from->str()) && from->getKnownIntValue() != 0) {
+            std::string format;
+            if (MathLib::isDec(from->str()))
+                format = "decimal";
+            else if (MathLib::isOct(from->str()))
+                format = "octal";
+            else
+                continue;
+            intToPointerCastError(tok, format);
+        }
+    }
+}
+
+void CheckOther::intToPointerCastError(const Token *tok, const std::string& format)
+{
+    reportError(tok, Severity::portability, "intToPointerCast",
+                "Casting non-zero " + format + " integer literal to pointer.",
+                CWE398, Certainty::normal);
+}
+
 void CheckOther::suspiciousFloatingPointCast()
 {
     if (!mSettings->severity.isEnabled(Severity::style) && !mSettings->isPremiumEnabled("suspiciousFloatingPointCast"))
@@ -4393,6 +4507,8 @@ void CheckOther::runChecks(const Tokenizer &tokenizer, ErrorLogger *errorLogger)
 
     // Checks
     checkOther.warningOldStylePointerCast();
+    checkOther.warningDangerousTypeCast();
+    checkOther.warningIntToPointerCast();
     checkOther.suspiciousFloatingPointCast();
     checkOther.invalidPointerCast();
     checkOther.checkCharVariable();
@@ -4459,6 +4575,8 @@ void CheckOther::getErrorMessages(ErrorLogger *errorLogger, const Settings *sett
     c.checkComparisonFunctionIsAlwaysTrueOrFalseError(nullptr, "isless","varName",false);
     c.checkCastIntToCharAndBackError(nullptr, "func_name");
     c.cstyleCastError(nullptr);
+    c.dangerousTypeCastError(nullptr, true);
+    c.intToPointerCastError(nullptr, "decimal");
     c.suspiciousFloatingPointCastError(nullptr);
     c.passedByValueError(nullptr, false);
     c.constVariableError(nullptr, nullptr);

lib/checkother.h

@@ -80,6 +80,12 @@ class CPPCHECKLIB CheckOther : public Check {
     /** @brief Are there C-style pointer casts in a c++ file? */
     void warningOldStylePointerCast();
 
+    /** @brief Dangerous type cast */
+    void warningDangerousTypeCast();
+
+    /** @brief Casting non-hexadecimal integer literal to pointer */
+    void warningIntToPointerCast();
+
     void suspiciousFloatingPointCast();
 
     /** @brief Check for pointer casts to a type with an incompatible binary data representation */
@@ -198,6 +204,8 @@ class CPPCHECKLIB CheckOther : public Check {
     void clarifyCalculationError(const Token *tok, const std::string &op);
     void clarifyStatementError(const Token* tok);
     void cstyleCastError(const Token *tok, bool isPtr = true);
+    void dangerousTypeCastError(const Token *tok, bool isPtr);
+    void intToPointerCastError(const Token *tok, const std::string& format);
     void suspiciousFloatingPointCastError(const Token *tok);
     void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive, bool toIsInt);
     void passedByValueError(const Variable* var, bool inconclusive, bool isRangeBasedFor = false);
@@ -273,6 +281,7 @@ class CPPCHECKLIB CheckOther : public Check {
                // warning
                "- either division by zero or useless condition\n"
                "- access of moved or forwarded variable.\n"
+               "- potentially dangerous C style type cast of pointer/reference to object.\n"
 
                // performance
                "- redundant data copying for const variable\n"
@@ -281,6 +290,7 @@ class CPPCHECKLIB CheckOther : public Check {
 
                // portability
                "- Passing NULL pointer to function with variable number of arguments leads to UB.\n"
+               "- Casting non-zero integer literal in decimal or octal format to pointer.\n"
 
                // style
                "- C-style pointer cast in C++ code\n"

man/checkers/cstyleCast.md

@@ -0,0 +1,50 @@
+
+# cstyleCast
+
+**Message**: C-style pointer casting<br/>
+**Category**: Modernization<br/>
+**Severity**: Style<br/>
+**Language**: C++, not applicable for C code
+
+## Description
+
+Many developers feel that it's best to replace C casts with C++ casts.
+
+There are several advantages with C++ casts:
+ * they permit only certain conversions
+ * they express the intent
+ * they are easy to identify
+
+This checker is about C casts that converts to/from a pointer or reference.
+
+**Note:** More "noisy" warnings exists that warn about all C casts. For instance Clang has
+`-Wold-style-cast` and there is also such checking in Misra C++.
+
+Dangerous conversions are covered by other warnings so this ID `cstyleCast` is primarily about
+writing warnings for casts that are currently safe.
+
+## How to fix
+
+You can use C++ casts such as `static_cast` to fix these warnings.
+
+The `dynamic_cast` should rarelly be used to fix these warnings because dangerousTypeCast is
+reported when that can be a good idea.
+
+Before:
+```cpp
+struct Base{};
+struct Derived: public Base {};
+void foo(Base* base) {
+    Base *p = (Base*)derived; // <- cstyleCast, cast from derived object to base object is safe now
+}
+```
+
+After:
+```cpp
+struct Base{};
+struct Derived: public Base {};
+void foo(Base* base) {
+    Derived *p = static_cast<Derived*>(base);
+}
+```
+The `static_cast` ensures that there will not be loss of constness in the future.

man/checkers/dangerousTypeCast.md

@@ -0,0 +1,43 @@
+
+# dangerousTypeCast
+
+**Message**: Potentially invalid type conversion in old-style C cast, clarify/fix with C++ cast<br/>
+**Category**: Type Safety<br/>
+**Severity**: Warning<br/>
+**Language**: C++, not applicable for C code
+
+## Motivation
+
+C style casts can be dangerous in many ways:
+ * loss of precision
+ * loss of sign
+ * loss of constness
+ * invalid type conversion
+
+## Philosophy
+
+This checker warns about old style C casts that perform type conversions that can be invalid.
+
+This checker is not about readability. It is about safety.
+
+## How to fix
+
+You can use `dynamic_cast` or `static_cast` to fix these warnings.
+
+Before:
+```cpp
+struct Base{};
+struct Derived: public Base {};
+void foo(Base* base) {
+    Derived *p = (Derived*)base; // <- can be invalid
+}
+```
+
+After:
+```cpp
+struct Base{};
+struct Derived: public Base {};
+void foo(Base* base) {
+    Derived *p = dynamic_cast<Derived*>(base);
+}
+```

releasenotes.txt

@@ -25,4 +25,5 @@ Other:
 - CMake will now unconditionally use Boost.Containers if available. If CMake option `USE_BOOST` is specified it will now bail out when it is not found.
 - Fix checking a project that contains several project file entries for the same file.
 - Fixed --file-filter matching of looked up files in provided paths.
+- Split up cstyleCast checker; dangerous casts produce portability/warning reports, safe casts produce style reports.
 -

test/cfg/posix.c

@@ -144,6 +144,7 @@ void nullPointer_pthread_attr_setstack(const pthread_attr_t *attr) {
     (void) pthread_attr_setstack(NULL, NULL, 0);
     (void) pthread_attr_setstack(attr, NULL, 0);
     // cppcheck-suppress nullPointer
+    // cppcheck-suppress intToPointerCast
     (void) pthread_attr_setstack(NULL, (void*) 1, 0);
 }
 

test/cfg/windows.cpp

@@ -825,7 +825,7 @@ void invalidFunctionArg()
     CloseHandle(hMutex);
 
     //Incorrect: 2. parameter to LoadLibraryEx() must be NULL
-    // cppcheck-suppress [invalidFunctionArg, cstyleCast]
+    // cppcheck-suppress [invalidFunctionArg, intToPointerCast]
     HINSTANCE hInstLib = LoadLibraryEx(L"My.dll", HANDLE(1), 0);
     FreeLibrary(hInstLib);