From 9eb16e10020918d725992f67b07c860d9b266478 Mon Sep 17 00:00:00 2001
From: PKEuS <731902+PKEuS@users.noreply.github.com>
Date: Sun, 19 Jun 2022 12:01:55 +0200
Subject: [PATCH] Replaced check for pipe() buffer size by ordinary
 CheckBufferOverrun, provide required Library configuration option (#4183)

Merged from LCppC.
---
 cfg/cppcheck-cfg.rng       |   3 +
 cfg/gnu.cfg                |   2 +-
 cfg/posix.cfg              |  10 ++--
 lib/checkbufferoverrun.cpp |  13 +++--
 lib/checkother.cpp         |  41 --------------
 lib/checkother.h           |   7 ---
 lib/library.cpp            |   3 +
 lib/library.h              |   1 +
 lib/tokenize.cpp           |  13 +++++
 lib/tokenize.h             |   3 +-
 test/testbufferoverrun.cpp |  40 ++++++++++++++
 test/testlibrary.cpp       |  15 ++++-
 test/testother.cpp         | 109 -------------------------------------
 13 files changed, 91 insertions(+), 169 deletions(-)

diff --git a/cfg/cppcheck-cfg.rng b/cfg/cppcheck-cfg.rng
index ef5a30907..b81a35335 100644
--- 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
index 575c4c8cb..75823db77 100644
--- 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
index 6de3b6282..083cf12b7 100644
--- 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
index 3ec9af3c9..499813323 100644
--- 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
index 88431816b..b31f2cdf3 100644
--- 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
index f9e0b44e3..8ef99473a 100644
--- a/lib/checkother.h
+++ b/lib/checkother.h
@@ -87,7 +87,6 @@ public:
         checkOther.checkKnownArgument();
         checkOther.checkComparePointers();
         checkOther.checkIncompleteStatement();
-        checkOther.checkPipeParameterSize();
         checkOther.checkRedundantCopy();
         checkOther.clarifyCalculation();
         checkOther.checkPassByReference();
@@ -190,9 +189,6 @@ public:
     /** @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 @@ private:
     // 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 @@ private:
         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 @@ private:
                "- 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
index 16c471709..c68d55979 100644
--- 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
index 3938b9bdc..f05c00990 100644
--- a/lib/library.h
+++ b/lib/library.h
@@ -327,6 +327,7 @@ public:
             int arg;
             int arg2;
             long long value;
+            std::string baseType;
         };
         std::vector<MinSize> minsizes;
 
diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp
index e7ba689be..aa67e97bf 100644
--- 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
index a123f6620..eb71d4036 100644
--- a/lib/tokenize.h
+++ b/lib/tokenize.h
@@ -181,7 +181,8 @@ public:
      * @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
index aa62c26b8..109944fe3 100644
--- a/test/testbufferoverrun.cpp
+++ b/test/testbufferoverrun.cpp
@@ -322,6 +322,8 @@ private:
         TEST_CASE(ctu_arithmetic);
 
         TEST_CASE(objectIndex);
+
+        TEST_CASE(checkPipeParameterSize); // ticket #3521
     }
 
 
@@ -5227,6 +5229,44 @@ private:
               "}\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
index c1975fff6..6bf72d0de 100644
--- a/test/testlibrary.cpp
+++ b/test/testlibrary.cpp
@@ -481,6 +481,7 @@ private:
                                "    <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>";
 
@@ -488,7 +489,7 @@ private:
         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 @@ private:
             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
index 7389c3b9b..4af565858 100644
--- a/test/testother.cpp
+++ b/test/testother.cpp
@@ -208,8 +208,6 @@ private:
 
         TEST_CASE(varFuncNullUB);
 
-        TEST_CASE(checkPipeParameterSize); // ticket #3521
-
         TEST_CASE(checkCastIntToCharAndBack); // ticket #160
 
         TEST_CASE(checkCommaSeparatedReturn);
@@ -350,20 +348,6 @@ private:
         checkOther.runChecks(&tokenizer, settings, this);
     }
 
-    void checkposix(const char code[]) {
-        static Settings settings;
-        settings.severity.enable(Severity::warning);
-        settings.libraries.emplace_back("posix");
-
-        check(code,
-              nullptr, // filename
-              false,   // experimental
-              false,   // inconclusive
-              true,    // runSimpleChecks
-              false,   // verbose
-              &settings);
-    }
-
     void checkInterlockedDecrement(const char code[]) {
         static Settings settings;
         settings.platformType = Settings::Win32A;
@@ -8627,99 +8611,6 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
-    void checkPipeParameterSize() { // #3521
-
-        checkposix("void f(){\n"
-                   "int pipefd[1];\n" // <--  array of two integers is needed
-                   "if (pipe(pipefd) == -1) {\n"
-                   "    return;\n"
-                   "  }\n"
-                   "}");
-        ASSERT_EQUALS("[test.cpp:3]: (error) Buffer 'pipefd' must have size of 2 integers if used as parameter of pipe().\n", errout.str());
-
-        checkposix("void f(){\n"
-                   "int pipefd[2];\n"
-                   "if (pipe(pipefd) == -1) {\n"
-                   "    return;\n"
-                   "  }\n"
-                   "}");
-        ASSERT_EQUALS("", errout.str());
-
-        checkposix("void f(){\n"
-                   "int pipefd[20];\n"
-                   "if (pipe(pipefd) == -1) {\n"
-                   "    return;\n"
-                   "  }\n"
-                   "}");
-        ASSERT_EQUALS("", errout.str());
-
-        checkposix("void f(){\n"
-                   "int pipefd[1];\n" // <--  array of two integers is needed
-                   "if (pipe2(pipefd,0) == -1) {\n"
-                   "    return;\n"
-                   "  }\n"
-                   "}");
-        ASSERT_EQUALS("[test.cpp:3]: (error) Buffer 'pipefd' must have size of 2 integers if used as parameter of pipe().\n", errout.str());
-
-        checkposix("void f(){\n"
-                   "int pipefd[2];\n"
-                   "if (pipe2(pipefd,0) == -1) {\n"
-                   "    return;\n"
-                   "  }\n"
-                   "}");
-        ASSERT_EQUALS("", errout.str());
-
-        checkposix("void f(){\n"
-                   "int pipefd[20];\n"
-                   "if (pipe2(pipefd,0) == -1) {\n"
-                   "    return;\n"
-                   "  }\n"
-                   "}");
-        ASSERT_EQUALS("", errout.str());
-
-        // avoid crash with pointer variable
-        check("void foo (int* arrayPtr)\n"
-              "{\n"
-              "  if (pipe (arrayPtr) < 0)\n"
-              "  {}\n"
-              "}");
-        ASSERT_EQUALS("", errout.str());
-
-        // avoid crash with pointer variable - for local variable on stack as well - see #4801
-        check("void foo() {\n"
-              "  int *cp;\n"
-              "  if ( pipe (cp) == -1 ) {\n"
-              "     return;\n"
-              "  }\n"
-              "}");
-        ASSERT_EQUALS("", errout.str());
-
-        // test with unknown variable
-        check("void foo() {\n"
-              "  if ( pipe (cp) == -1 ) {\n"
-              "     return;\n"
-              "  }\n"
-              "}");
-        ASSERT_EQUALS("", errout.str());
-
-        // avoid crash with pointer variable - for local variable on stack as well - see #4801
-        check("void foo() {\n"
-              "  int *cp;\n"
-              "  if ( pipe (cp) == -1 ) {\n"
-              "     return;\n"
-              "  }\n"
-              "}");
-        ASSERT_EQUALS("", errout.str());
-
-        // test with unknown variable
-        check("void foo() {\n"
-              "  if ( pipe (cp) == -1 ) {\n"
-              "     return;\n"
-              "  }\n"
-              "}");
-        ASSERT_EQUALS("", errout.str());
-    }
-
     void checkCastIntToCharAndBack() { // #160
 
         // check getchar