Fixed #4482 (add test for UB due to usage of NULL in variadic functions)

lib/checkother.cpp

@@ -3748,3 +3748,85 @@ void CheckOther::oppositeInnerConditionError(const Token *tok)
 {
     reportError(tok, Severity::warning, "oppositeInnerCondition", "Opposite conditions in nested 'if' blocks lead to a dead code block.", true);
 }
+
+
+void CheckOther::checkVarFuncNullUB()
+{
+    if (!_settings->isEnabled("portability"))
+        return;
+
+    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
+    const std::size_t functions = symbolDatabase->functionScopes.size();
+    for (std::size_t i = 0; i < functions; ++i) {
+        const Scope * scope = symbolDatabase->functionScopes[i];
+        for (const Token* tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) {
+            // Is NULL passed to a function?
+            if (Token::Match(tok,"[(,] NULL [,)]")) {
+                // Locate function name in this function call.
+                const Token *ftok = tok;
+                while (ftok && ftok->str() != "(") {
+                    if (ftok->str() == ")")
+                        ftok = ftok->link();
+                    ftok = ftok->previous();
+                }
+                ftok = ftok ? ftok->previous() : NULL;
+                if (ftok && ftok->isName()) {
+                    // If this is a variadic function then report error
+                    const Function *f = symbolDatabase->findFunctionByName(ftok->str(), scope);
+                    if (f) {
+                        const Token *tok2 = f->argDef;
+                        tok2 = tok2 ? tok2->link() : NULL; // goto ')'
+                        if (Token::simpleMatch(tok2->tokAt(-3), ". . ."))
+                            varFuncNullUBError(tok);
+                    }
+                }
+            }
+        }
+    }
+}
+
+void CheckOther::varFuncNullUBError(const Token *tok)
+{
+    reportError(tok,
+                Severity::portability,
+                "varFuncNullUB",
+                "Passing NULL to a function with variable number of arguments leads to undefined behaviour on some platforms.\n"
+                "Passing NULL to a function with variable number of arguments leads to undefined behaviour on some platforms.\n"
+                "The behaviour is undefined when NULL is #defined as 0, sizeof(int)!=sizeof(void*) and the function expects a pointer. Otherwise the behaviour is defined.\n"
+                "See section section 7.1.4 and section 7.15.1.1 in the C standard. Section 7.1.4 explains that the function call is UB. Section 7.15.1.1 explains that the va_arg macro has UB.\n"
+                "To reproduce you might be able to use this little code example. Try it on a platform where sizeof(int)!=sizeof(void*), for instance on a x86_64 machine. If ERROR is written by the program on the screen it means that 0 is not converted to a NULL pointer. Changing the 0 to (void*)0 will fix the program.\n"
+                "#include <stdarg.h>\n"
+                "#include <stdio.h>\n"
+                "\n"
+                "void f(char *s, ...) {\n"
+                "    va_list ap;\n"
+                "    va_start(ap,s);\n"
+                "    for (;;) {\n"
+                "        char *p = va_arg(ap,char*);\n"
+                "        printf(\"%018p, %s\n\", p, (long)p & 255 ? p : \"\");\n"
+                "        if(!p) break;\n"
+                "    }\n"
+                "    va_end(ap);\n"
+                "}\n"
+                "\n"
+                "void g() {\n"
+                "    char *s2 = \"x\";\n"
+                "   char *s3 = \"ERROR\";\n"
+                "\n"
+                "    // changing 0 to 0L makes the error go away on x86_64\n"
+                "    f(\"first\", s2, s2, s2, s2, s2, 0, s3, (char*)0);\n"
+                "}\n"
+                "\n"
+                "void h() {\n"
+                "    int i;\n"
+                "    volatile unsigned char a[1000];\n"
+                "    for (i = 0; i<sizeof(a); i++)\n"
+                "        a[i] = -1;\n"
+                "}\n"
+                "\n"
+                "int main() {\n"
+                "    h();\n"
+                "    g();\n"
+                "    return 0;\n"
+                "}");
+}

lib/checkother.h

@@ -80,6 +80,7 @@ public:
         checkOther.checkSignOfUnsignedVariable();  // don't ignore casts (#3574)
         checkOther.checkIncompleteArrayFill();
         checkOther.checkSuspiciousStringCompare();
+        checkOther.checkVarFuncNullUB();
     }
 
     /** @brief Run checks against the simplified token list */
@@ -285,6 +286,9 @@ public:
     /** @brief %Check for buffers that are filled incompletely with memset and similar functions */
     void checkIncompleteArrayFill();
 
+    /** @brief %Check that variadic function calls don't use NULL. If NULL is #defined as 0 and the function expects a pointer, the behaviour is undefined. */
+    void checkVarFuncNullUB();
+
 private:
     // Error messages..
     void oppositeInnerConditionError(const Token *tok);
@@ -354,6 +358,7 @@ private:
     void negativeBitwiseShiftError(const Token *tok);
     void redundantCopyError(const Token *tok, const std::string &varname);
     void incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean);
+    void varFuncNullUBError(const Token *tok);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckOther c(0, settings, errorLogger);
@@ -428,6 +433,7 @@ private:
         c.cctypefunctionCallError(0, "funname", "value");
         c.moduloAlwaysTrueFalseError(0, "1");
         c.incompleteArrayFillError(0, "buffer", "memset", false);
+        c.varFuncNullUBError(0);
     }
 
     static std::string myName() {
@@ -495,7 +501,8 @@ private:
                "* Suspicious use of ; at the end of 'if/for/while' statement.\n"
                "* incorrect usage of functions from ctype library.\n"
                "* Comparisons of modulo results that are always true/false.\n"
-               "* Array filled incompletely using memset/memcpy/memmove.\n";
+               "* Array filled incompletely using memset/memcpy/memmove.\n"
+               "* Passing NULL pointer to function with variable number of arguments leads to UB on some platforms.\n";
     }
 
     void checkExpressionRange(const std::list<const Function*> &constFunctions,

lib/tokenize.cpp

@@ -2131,14 +2131,14 @@ void Tokenizer::simplifyFileAndLineMacro()
 void Tokenizer::simplifyNull()
 {
     for (Token *tok = list.front(); tok; tok = tok->next()) {
-        if (tok->str() == "NULL" || tok->str() == "__null" ||
+        if (tok->str() == "NULL" && !Token::Match(tok->previous(), "[(,] NULL [,)]"))
-            tok->str() == "'\\0'" || tok->str() == "'\\x0'") {
             tok->str("0");
-        } else if (tok->isNumber() &&
+        else if (tok->str() == "__null" || tok->str() == "'\\0'" || tok->str() == "'\\x0'")
-                   MathLib::isInt(tok->str()) &&
+            tok->str("0");
-                   MathLib::toLongNumber(tok->str()) == 0) {
+        else if (tok->isNumber() &&
+                 MathLib::isInt(tok->str()) &&
+                 MathLib::toLongNumber(tok->str()) == 0)
             tok->str("0");
-        }
     }
 
     // nullptr..

test/testother.cpp

@@ -188,6 +188,8 @@ private:
 
         TEST_CASE(redundantVarAssignment);
         TEST_CASE(redundantMemWrite);
+
+        TEST_CASE(varFuncNullUB);
     }
 
     void check(const char code[], const char *filename = NULL, bool experimental = false, bool inconclusive = true) {
@@ -6799,6 +6801,12 @@ private:
               "}");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void varFuncNullUB() { // #4482
+        check("void a(...);\n"
+              "void b() { a(NULL); }");
+        ASSERT_EQUALS("[test.cpp:2]: (portability) Passing NULL to a function with variable number of arguments leads to undefined behaviour on some platforms.\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestOther)

test/testsimplifytokens.cpp

@@ -3407,7 +3407,7 @@ private:
                 ASSERT_EQUALS("void f ( int i ) { goto label ; { label : ; " + *it + " ; } }",tok("void f (int i) { goto label; switch(i) { label: " + *it + "; } }"));
                 //ticket #3148
                 ASSERT_EQUALS("void f ( ) { MACRO ( " + *it + " ) }",tok("void f() { MACRO(" + *it + ") }"));
-                ASSERT_EQUALS("void f ( ) { MACRO ( " + *it + " ; , 0 ) }",tok("void f() { MACRO(" + *it + ";, NULL) }"));
+                ASSERT_EQUALS("void f ( ) { MACRO ( " + *it + " ; , NULL ) }",tok("void f() { MACRO(" + *it + ";, NULL) }"));
                 ASSERT_EQUALS("void f ( ) { MACRO ( bar1 , " + *it + " ) }",tok("void f() { MACRO(bar1, " + *it + ") }"));
                 ASSERT_EQUALS("void f ( ) { MACRO ( " + *it + " ; bar2 , foo ) }",tok("void f() { MACRO(" + *it + "; bar2, foo) }"));
             }

test/testtokenize.cpp

@@ -448,6 +448,8 @@ private:
         TEST_CASE(simplifyOperatorName5);
         TEST_CASE(simplifyOperatorName6); // ticket #3194
 
+        TEST_CASE(simplifyNull);
+
         TEST_CASE(simplifyNullArray);
 
         // Some simple cleanups of unhandled macros in the global scope
@@ -7207,6 +7209,11 @@ private:
         ASSERT_EQUALS(result2, tokenizeAndStringify(code2,false));
     }
 
+    void simplifyNull() {
+        ASSERT_EQUALS("if ( p == 0 )", tokenizeAndStringify("if (p==NULL)"));
+        ASSERT_EQUALS("f ( NULL ) ;", tokenizeAndStringify("f(NULL);"));
+    }
+
     void simplifyNullArray() {
         ASSERT_EQUALS("* ( foo . bar [ 5 ] ) = x ;", tokenizeAndStringify("0[foo.bar[5]] = x;"));
     }