Replaced check for pipe() buffer size by ordinary CheckBufferOverrun, provide required Library configuration option (#4183)

PKEuS · web-flow · commit 9eb16e100209 · 2022-06-19T12:01:55.000+02:00
Merged from LCppC.
diff --git a/cfg/cppcheck-cfg.rng b/cfg/cppcheck-cfg.rng
@@ -306,6 +306,9 @@
                       <attribute name="value">
                         <ref name="MINSIZE-VALUE"/>
                       </attribute>
+                      <optional>
+                        <attribute name="baseType"><text/></attribute>
+                      </optional>
                     </element>
                   </choice>
                 </zeroOrMore>
diff --git a/cfg/gnu.cfg b/cfg/gnu.cfg
@@ -414,7 +414,7 @@
     <arg nr="1" direction="out">
       <not-null/>
       <not-bool/>
-      <minsize type="value" value="2"/>
+      <minsize type="value" value="2" baseType="int"/>
     </arg>
     <arg nr="2" direction="in">
       <not-uninit/>
diff --git a/cfg/posix.cfg b/cfg/posix.cfg
@@ -1742,7 +1742,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
     <arg nr="2" direction="in">
       <not-uninit/>
       <not-bool/>
-      <minsize type="value" value="2"/>
+      <minsize type="value" value="2" baseType="timespec"/>
     </arg>
   </function>
   <!-- int utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags);-->
@@ -1762,7 +1762,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
     <arg nr="3" direction="in">
       <not-uninit/>
       <not-bool/>
-      <minsize type="value" value="2"/>
+      <minsize type="value" value="2" baseType="timespec"/>
     </arg>
     <arg nr="4" direction="in">
       <not-uninit/>
@@ -1782,7 +1782,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
     <arg nr="2" direction="in">
       <not-uninit/>
       <not-bool/>
-      <minsize type="value" value="2"/>
+      <minsize type="value" value="2" baseType="timeval"/>
     </arg>
     <warn severity="style" reason="Obsolescent" alternatives="utimensat"/>
   </function>
@@ -2749,7 +2749,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
     <arg nr="4" direction="out">
       <not-null/>
       <not-bool/>
-      <minsize type="value" value="2"/>
+      <minsize type="value" value="2" baseType="int"/>
     </arg>
   </function>
   <!-- http://man7.org/linux/man-pages/man2/socketpair.2.html -->
@@ -2761,7 +2761,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
     <arg nr="1" direction="out">
       <not-null/>
       <not-bool/>
-      <minsize type="value" value="2"/>
+      <minsize type="value" value="2" baseType="int"/>
     </arg>
   </function>
   <!-- int pselect(int nfds, fd_set *restrict readfds,
diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp
@@ -563,7 +563,7 @@ ValueFlow::Value CheckBufferOverrun::getBufferSize(const Token *bufTok) const
 }
 //---------------------------------------------------------------------------
 
-static bool checkBufferSize(const Token *ftok, const Library::ArgumentChecks::MinSize &minsize, const std::vector<const Token *> &args, const MathLib::bigint bufferSize, const Settings *settings)
+static bool checkBufferSize(const Token *ftok, const Library::ArgumentChecks::MinSize &minsize, const std::vector<const Token *> &args, const MathLib::bigint bufferSize, const Settings *settings, const Tokenizer* tokenizer)
 {
     const Token * const arg = (minsize.arg > 0 && minsize.arg - 1 < args.size()) ? args[minsize.arg - 1] : nullptr;
     const Token * const arg2 = (minsize.arg2 > 0 && minsize.arg2 - 1 < args.size()) ? args[minsize.arg2 - 1] : nullptr;
@@ -589,8 +589,13 @@ static bool checkBufferSize(const Token *ftok, const Library::ArgumentChecks::Mi
         if (arg && arg2 && arg->hasKnownIntValue() && arg2->hasKnownIntValue())
             return (arg->getKnownIntValue() * arg2->getKnownIntValue()) <= bufferSize;
         break;
-    case Library::ArgumentChecks::MinSize::Type::VALUE:
-        return minsize.value <= bufferSize;
+    case Library::ArgumentChecks::MinSize::Type::VALUE: {
+        MathLib::bigint myMinsize = minsize.value;
+        unsigned int baseSize = tokenizer->sizeOfType(minsize.baseType);
+        if (baseSize != 0)
+            myMinsize *= baseSize;
+        return myMinsize <= bufferSize;
+    }
     case Library::ArgumentChecks::MinSize::Type::NONE:
         break;
     }
@@ -644,7 +649,7 @@ void CheckBufferOverrun::bufferOverflow()
                     }
                 }
                 const bool error = std::none_of(minsizes->begin(), minsizes->end(), [=](const Library::ArgumentChecks::MinSize &minsize) {
-                    return checkBufferSize(tok, minsize, args, bufferSize.intvalue, mSettings);
+                    return checkBufferSize(tok, minsize, args, bufferSize.intvalue, mSettings, mTokenizer);
                 });
                 if (error)
                     bufferOverflowError(args[argnr], &bufferSize, (bufferSize.intvalue == 1) ? Certainty::inconclusive : Certainty::normal);
diff --git a/lib/checkother.cpp b/lib/checkother.cpp
@@ -384,47 +384,6 @@ void CheckOther::invalidPointerCastError(const Token* tok, const std::string& fr
         reportError(tok, Severity::portability, "invalidPointerCast", "Casting between " + from + " and " + to + " which have an incompatible binary data representation.", CWE704, Certainty::normal);
 }
 
-//---------------------------------------------------------------------------
-// This check detects errors on POSIX systems, when a pipe command called
-// with a wrong dimensioned file descriptor array. The pipe command requires
-// exactly an integer array of dimension two as parameter.
-//
-// References:
-//  - http://linux.die.net/man/2/pipe
-//  - ticket #3521
-//---------------------------------------------------------------------------
-void CheckOther::checkPipeParameterSize()
-{
-    if (!mSettings->posix())
-        return;
-
-    const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
-    for (const Scope * scope : symbolDatabase->functionScopes) {
-        for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) {
-            if (Token::Match(tok, "pipe ( %var% )") ||
-                Token::Match(tok, "pipe2 ( %var% ,")) {
-                const Token * const varTok = tok->tokAt(2);
-
-                const Variable *var = varTok->variable();
-                MathLib::bigint dim;
-                if (var && var->isArray() && !var->isArgument() && ((dim=var->dimension(0U)) < 2)) {
-                    const std::string strDim = MathLib::toString(dim);
-                    checkPipeParameterSizeError(varTok,varTok->str(), strDim);
-                }
-            }
-        }
-    }
-}
-
-void CheckOther::checkPipeParameterSizeError(const Token *tok, const std::string &strVarName, const std::string &strDim)
-{
-    reportError(tok, Severity::error,
-                "wrongPipeParameterSize",
-                "$symbol:" + strVarName + "\n"
-                "Buffer '$symbol' must have size of 2 integers if used as parameter of pipe().\n"
-                "The pipe()/pipe2() system command takes an argument, which is an array of exactly two integers.\n"
-                "The variable '$symbol' is an array of size " + strDim + ", which does not match.", CWE686, Certainty::safe);
-}
 
 //---------------------------------------------------------------------------
 // Detect redundant assignments: x = 0; x = 4;
diff --git a/lib/checkother.h b/lib/checkother.h
@@ -87,7 +87,6 @@ class CPPCHECKLIB CheckOther : public Check {
         checkOther.checkKnownArgument();
         checkOther.checkComparePointers();
         checkOther.checkIncompleteStatement();
-        checkOther.checkPipeParameterSize();
         checkOther.checkRedundantCopy();
         checkOther.clarifyCalculation();
         checkOther.checkPassByReference();
@@ -190,9 +189,6 @@ class CPPCHECKLIB CheckOther : public Check {
     /** @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();
 
-    /** @brief %Check that calling the POSIX pipe() system call is called with an integer array of size two. */
-    void checkPipeParameterSize();
-
     /** @brief %Check to avoid casting a return value to unsigned char and then back to integer type.  */
     void checkCastIntToCharAndBack();
 
@@ -234,7 +230,6 @@ class CPPCHECKLIB CheckOther : public Check {
     // Error messages..
     void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, const bool result);
     void checkCastIntToCharAndBackError(const Token *tok, const std::string &strFunctionName);
-    void checkPipeParameterSizeError(const Token *tok, const std::string &strVarName, const std::string &strDim);
     void clarifyCalculationError(const Token *tok, const std::string &op);
     void clarifyStatementError(const Token* tok);
     void cstyleCastError(const Token *tok);
@@ -299,7 +294,6 @@ class CPPCHECKLIB CheckOther : public Check {
         c.invalidPointerCastError(nullptr,  "float *", "double *", false, false);
         c.negativeBitwiseShiftError(nullptr, 1);
         c.negativeBitwiseShiftError(nullptr, 2);
-        c.checkPipeParameterSizeError(nullptr,  "varname", "dimension");
         c.raceAfterInterlockedDecrementError(nullptr);
         c.invalidFreeError(nullptr, "malloc", false);
         c.overlappingWriteUnion(nullptr);
@@ -378,7 +372,6 @@ class CPPCHECKLIB CheckOther : public Check {
                "- assignment in an assert statement\n"
                "- free() or delete of an invalid memory location\n"
                "- bitwise operation with negative right operand\n"
-               "- provide wrong dimensioned array to pipe() system command (--std=posix)\n"
                "- cast the return values of getc(),fgetc() and getchar() to character and compare it to EOF\n"
                "- race condition with non-interlocked access after InterlockedDecrement() call\n"
                "- expression 'x = x++;' depends on order of evaluation of side effects\n"
diff --git a/lib/library.cpp b/lib/library.cpp
@@ -782,6 +782,9 @@ Library::Error Library::loadFunction(const tinyxml2::XMLElement * const node, co
                             return Error(ErrorCode::BAD_ATTRIBUTE_VALUE, valueattr);
                         ac.minsizes.emplace_back(type, 0);
                         ac.minsizes.back().value = minsizevalue;
+                        const char* baseTypeAttr = argnode->Attribute("baseType");
+                        if (baseTypeAttr)
+                            ac.minsizes.back().baseType = baseTypeAttr;
                     } else {
                         const char *argattr = argnode->Attribute("arg");
                         if (!argattr)
diff --git a/lib/library.h b/lib/library.h
@@ -327,6 +327,7 @@ class CPPCHECKLIB Library {
             int arg;
             int arg2;
             long long value;
+            std::string baseType;
         };
         std::vector<MinSize> minsizes;
 
diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp
@@ -203,6 +203,19 @@ Tokenizer::~Tokenizer()
 // SizeOfType - gives the size of a type
 //---------------------------------------------------------------------------
 
+nonneg int Tokenizer::sizeOfType(const std::string& type) const
+{
+    const std::map<std::string, int>::const_iterator it = mTypeSize.find(type);
+    if (it == mTypeSize.end()) {
+        const Library::PodType* podtype = mSettings->library.podtype(type);
+        if (!podtype)
+            return 0;
+
+        return podtype->size;
+    }
+    return it->second;
+}
+
 nonneg int Tokenizer::sizeOfType(const Token *type) const
 {
     if (!type || type->str().empty())
diff --git a/lib/tokenize.h b/lib/tokenize.h
@@ -181,7 +181,8 @@ class CPPCHECKLIB Tokenizer {
      * @param type Token which will contain e.g. "int", "*", or string.
      * @return sizeof for given type, or 0 if it can't be calculated.
      */
-    nonneg int sizeOfType(const Token *type) const;
+    nonneg int sizeOfType(const Token* type) const;
+    nonneg int sizeOfType(const std::string& type) const;
 
     void simplifyDebug();
     /**
diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp
@@ -322,6 +322,8 @@ class TestBufferOverrun : public TestFixture {
         TEST_CASE(ctu_arithmetic);
 
         TEST_CASE(objectIndex);
+
+        TEST_CASE(checkPipeParameterSize); // ticket #3521
     }
 
 
@@ -5227,6 +5229,44 @@ class TestBufferOverrun : public TestFixture {
               "}\n");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void checkPipeParameterSize() { // #3521
+
+        Settings settings;
+        LOAD_LIB_2(settings.library, "posix.cfg");
+
+        check("void f(){\n"
+              "int pipefd[1];\n" // <--  array of two integers is needed
+              "if (pipe(pipefd) == -1) {\n"
+              "    return;\n"
+              "  }\n"
+              "}", settings);
+        ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds: pipefd\n", errout.str());
+
+        check("void f(){\n"
+              "int pipefd[2];\n"
+              "if (pipe(pipefd) == -1) {\n"
+              "    return;\n"
+              "  }\n"
+              "}", settings);
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f(){\n"
+              "char pipefd[2];\n"
+              "if (pipe((int*)pipefd) == -1) {\n"
+              "    return;\n"
+              "  }\n"
+              "}", settings);
+        ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds: (int*)pipefd\n", errout.str());
+
+        check("void f(){\n"
+              "char pipefd[20];\n" // Strange, but large enough
+              "if (pipe((int*)pipefd) == -1) {\n"
+              "    return;\n"
+              "  }\n"
+              "}", settings);
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestBufferOverrun)
diff --git a/test/testlibrary.cpp b/test/testlibrary.cpp
@@ -481,14 +481,15 @@ class TestLibrary : public TestFixture {
                                "    <arg nr=\"2\"><minsize type=\"argvalue\" arg=\"3\"/></arg>\n"
                                "    <arg nr=\"3\"/>\n"
                                "    <arg nr=\"4\"><minsize type=\"value\" value=\"500\"/></arg>\n"
+                               "    <arg nr=\"5\"><minsize type=\"value\" value=\"4\" baseType=\"int\"/></arg>\n"
                                "  </function>\n"
                                "</def>";
 
         Library library;
         ASSERT_EQUALS(true, Library::ErrorCode::OK == (readLibrary(library, xmldata)).errorcode);
 
         TokenList tokenList(nullptr);
-        std::istringstream istr("foo(a,b,c,d);");
+        std::istringstream istr("foo(a,b,c,d,e);");
         tokenList.createTokens(istr);
         tokenList.front()->next()->astOperand1(tokenList.front());
 
@@ -520,6 +521,18 @@ class TestLibrary : public TestFixture {
             const Library::ArgumentChecks::MinSize &m = minsizes->front();
             ASSERT(Library::ArgumentChecks::MinSize::Type::VALUE == m.type);
             ASSERT_EQUALS(500, m.value);
+            ASSERT_EQUALS(emptyString, m.baseType);
+        }
+
+        // arg5: type=value
+        minsizes = library.argminsizes(tokenList.front(), 5);
+        ASSERT_EQUALS(true, minsizes != nullptr);
+        ASSERT_EQUALS(1U, minsizes ? minsizes->size() : 1U);
+        if (minsizes && minsizes->size() == 1U) {
+            const Library::ArgumentChecks::MinSize& m = minsizes->front();
+            ASSERT(Library::ArgumentChecks::MinSize::Type::VALUE == m.type);
+            ASSERT_EQUALS(4, m.value);
+            ASSERT_EQUALS("int", m.baseType);
         }
     }
 
diff --git a/test/testother.cpp b/test/testother.cpp
