do not use string-to-integer conversions without error handling (#4906)

Author: firewave

.clang-tidy

@@ -1,5 +1,5 @@
 ---
-Checks:          '*,-abseil-*,-altera-*,-android-*,-boost-*,-cert-*,-cppcoreguidelines-*,-darwin-*,-fuchsia-*,-google-*,-hicpp-*,-linuxkernel-*,-llvm-*,-llvmlibc-*,-mpi-*,-objc-*,-openmp-*,-zircon-*,google-explicit-constructor,-readability-braces-around-statements,-readability-magic-numbers,-bugprone-macro-parentheses,-readability-isolate-declaration,-readability-function-size,-modernize-use-trailing-return-type,-readability-implicit-bool-conversion,-readability-uppercase-literal-suffix,-modernize-use-auto,-readability-else-after-return,-modernize-use-default-member-init,-readability-redundant-member-init,-modernize-avoid-c-arrays,-modernize-use-equals-default,-readability-container-size-empty,-bugprone-branch-clone,-bugprone-narrowing-conversions,-modernize-raw-string-literal,-readability-convert-member-functions-to-static,-modernize-loop-convert,-readability-const-return-type,-modernize-return-braced-init-list,-performance-inefficient-string-concatenation,-misc-throw-by-value-catch-by-reference,-readability-avoid-const-params-in-decls,-misc-non-private-member-variables-in-classes,-clang-analyzer-*,-bugprone-signed-char-misuse,-misc-no-recursion,-readability-use-anyofallof,-performance-no-automatic-move,-readability-function-cognitive-complexity,-readability-redundant-access-specifiers,-concurrency-mt-unsafe,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-readability-identifier-length,-readability-container-data-pointer,-bugprone-assignment-in-if-condition,-misc-const-correctness,-portability-std-allocator-const,-modernize-deprecated-ios-base-aliases,-bugprone-unchecked-optional-access,-modernize-replace-auto-ptr,-readability-identifier-naming,-portability-simd-intrinsics,-misc-use-anonymous-namespace'
+Checks:          '*,-abseil-*,-altera-*,-android-*,-boost-*,-cert-*,-cppcoreguidelines-*,-darwin-*,-fuchsia-*,-google-*,-hicpp-*,-linuxkernel-*,-llvm-*,-llvmlibc-*,-mpi-*,-objc-*,-openmp-*,-zircon-*,google-explicit-constructor,-readability-braces-around-statements,-readability-magic-numbers,-bugprone-macro-parentheses,-readability-isolate-declaration,-readability-function-size,-modernize-use-trailing-return-type,-readability-implicit-bool-conversion,-readability-uppercase-literal-suffix,-modernize-use-auto,-readability-else-after-return,-modernize-use-default-member-init,-readability-redundant-member-init,-modernize-avoid-c-arrays,-modernize-use-equals-default,-readability-container-size-empty,-bugprone-branch-clone,-bugprone-narrowing-conversions,-modernize-raw-string-literal,-readability-convert-member-functions-to-static,-modernize-loop-convert,-readability-const-return-type,-modernize-return-braced-init-list,-performance-inefficient-string-concatenation,-misc-throw-by-value-catch-by-reference,-readability-avoid-const-params-in-decls,-misc-non-private-member-variables-in-classes,-clang-analyzer-*,-bugprone-signed-char-misuse,-misc-no-recursion,-readability-use-anyofallof,-performance-no-automatic-move,-readability-function-cognitive-complexity,-readability-redundant-access-specifiers,-concurrency-mt-unsafe,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-readability-identifier-length,-readability-container-data-pointer,-bugprone-assignment-in-if-condition,-misc-const-correctness,-portability-std-allocator-const,-modernize-deprecated-ios-base-aliases,-bugprone-unchecked-optional-access,-modernize-replace-auto-ptr,-readability-identifier-naming,-portability-simd-intrinsics,-misc-use-anonymous-namespace,cert-err34-c'
 WarningsAsErrors: '*'
 HeaderFilterRegex: '(cli|gui|lib|oss-fuzz|test|triage)\/[a-z]+\.h'
 CheckOptions:

.selfcheck_suppressions

@@ -5,6 +5,7 @@ bitwiseOnBoolean
 # temporary suppressions - fix the warnings!
 simplifyUsing:lib/valueptr.h
 varid0:gui/projectfile.cpp
+templateInstantiation
 
 # warnings in Qt generated code we cannot fix
 symbolDatabaseWarning:gui/temp/moc_*.cpp

cli/cmdlineparser.cpp

@@ -260,7 +260,8 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
             }
 
             else if (std::strncmp(argv[i], "--checks-max-time=", 18) == 0) {
-                mSettings.checksMaxTime = std::atoi(argv[i] + 18);
+                if (!parseNumberArg(argv[i], 18, mSettings.checksMaxTime))
+                    return false;
             }
 
             else if (std::strcmp(argv[i], "--clang") == 0) {
@@ -286,6 +287,7 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
             }
 
             else if (std::strncmp(argv[i], "--cppcheck-build-dir=", 21) == 0) {
+                // TODO: bail out when the folder does not exist? will silently do nothing
                 mSettings.buildDir = Path::fromNativeSeparators(argv[i] + 21);
                 if (endsWith(mSettings.buildDir, '/'))
                     mSettings.buildDir.pop_back();
@@ -365,12 +367,8 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
 
             // --error-exitcode=1
             else if (std::strncmp(argv[i], "--error-exitcode=", 17) == 0) {
-                const std::string temp = argv[i]+17;
-                std::istringstream iss(temp);
-                if (!(iss >> mSettings.exitCode)) {
-                    printError("argument must be an integer. Try something like '--error-exitcode=1'.");
+                if (!parseNumberArg(argv[i], 17, mSettings.exitCode))
                     return false;
-                }
             }
 
             // Exception handling inside cppcheck client
@@ -505,18 +503,19 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
                 else
                     numberString = argv[i]+2;
 
-                std::istringstream iss(numberString);
-                if (!(iss >> mSettings.jobs)) {
-                    printError("argument to '-j' is not a number.");
+                unsigned int tmp;
+                std::string err;
+                if (!strToInt(numberString, tmp, &err)) {
+                    printError("argument to '-j' is not valid - " +  err + ".");
                     return false;
                 }
-
-                if (mSettings.jobs > 10000) {
+                if (tmp > 10000) {
                     // This limit is here just to catch typos. If someone has
                     // need for more jobs, this value should be increased.
                     printError("argument for '-j' is allowed to be 10000 at max.");
                     return false;
                 }
+                mSettings.jobs = tmp;
             }
 
 #ifdef THREADING_MODEL_FORK
@@ -538,11 +537,13 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
                 else
                     numberString = argv[i]+2;
 
-                std::istringstream iss(numberString);
-                if (!(iss >> mSettings.loadAverage)) {
-                    printError("argument to '-l' is not a number.");
+                int tmp;
+                std::string err;
+                if (!strToInt(numberString, tmp, &err)) {
+                    printError("argument to '-l' is not valid - " + err + ".");
                     return false;
                 }
+                mSettings.loadAverage = tmp;
             }
 #endif
 
@@ -577,37 +578,40 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
 
             // Set maximum number of #ifdef configurations to check
             else if (std::strncmp(argv[i], "--max-configs=", 14) == 0) {
-                mSettings.force = false;
-
-                std::istringstream iss(14+argv[i]);
-                if (!(iss >> mSettings.maxConfigs)) {
-                    printError("argument to '--max-configs=' is not a number.");
+                int tmp;
+                if (!parseNumberArg(argv[i], 14, tmp))
                     return false;
-                }
-
-                if (mSettings.maxConfigs < 1) {
+                if (tmp < 1) {
                     printError("argument to '--max-configs=' must be greater than 0.");
                     return false;
                 }
 
+                mSettings.maxConfigs = tmp;
+                mSettings.force = false;
                 maxconfigs = true;
             }
 
             // max ctu depth
-            else if (std::strncmp(argv[i], "--max-ctu-depth=", 16) == 0)
-                mSettings.maxCtuDepth = std::atoi(argv[i] + 16);
+            else if (std::strncmp(argv[i], "--max-ctu-depth=", 16) == 0) {
+                if (!parseNumberArg(argv[i], 16, mSettings.maxCtuDepth))
+                    return false;
+            }
 
             // Write results in file
             else if (std::strncmp(argv[i], "--output-file=", 14) == 0)
                 mSettings.outputFile = Path::simplifyPath(Path::fromNativeSeparators(argv[i] + 14));
 
             // Experimental: limit execution time for extended valueflow analysis. basic valueflow analysis
             // is always executed.
-            else if (std::strncmp(argv[i], "--performance-valueflow-max-time=", 33) == 0)
-                mSettings.performanceValueFlowMaxTime = std::atoi(argv[i] + 33);
+            else if (std::strncmp(argv[i], "--performance-valueflow-max-time=", 33) == 0) {
+                if (!parseNumberArg(argv[i], 33, mSettings.performanceValueFlowMaxTime, true))
+                    return false;
+            }
 
-            else if (std::strncmp(argv[i], "--performance-valueflow-max-if-count=", 37) == 0)
-                mSettings.performanceValueFlowMaxIfCount = std::atoi(argv[i] + 37);
+            else if (std::strncmp(argv[i], "--performance-valueflow-max-if-count=", 37) == 0) {
+                if (!parseNumberArg(argv[i], 37, mSettings.performanceValueFlowMaxIfCount, true))
+                    return false;
+            }
 
             // Specify platform
             else if (std::strncmp(argv[i], "--platform=", 11) == 0) {
@@ -941,26 +945,18 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
             }
 
             else if (std::strncmp(argv[i], "--template-max-time=", 20) == 0) {
-                mSettings.templateMaxTime = std::atoi(argv[i] + 20);
+                if (!parseNumberArg(argv[i], 20, mSettings.templateMaxTime))
+                    return false;
             }
 
             else if (std::strncmp(argv[i], "--typedef-max-time=", 19) == 0) {
-                mSettings.typedefMaxTime = std::atoi(argv[i] + 19);
+                if (!parseNumberArg(argv[i], 19, mSettings.typedefMaxTime))
+                    return false;
             }
 
             else if (std::strncmp(argv[i], "--valueflow-max-iterations=", 27) == 0) {
-                long tmp;
-                try {
-                    tmp = std::stol(argv[i] + 27);
-                } catch (const std::invalid_argument &) {
-                    printError("argument to '--valueflow-max-iteration' is invalid.");
+                if (!parseNumberArg(argv[i], 27, mSettings.valueFlowMaxIterations))
                     return false;
-                }
-                if (tmp < 0) {
-                    printError("argument to '--valueflow-max-iteration' needs to be at least 0.");
-                    return false;
-                }
-                mSettings.valueFlowMaxIterations = static_cast<std::size_t>(tmp);
             }
 
             else if (std::strcmp(argv[i], "-v") == 0 || std::strcmp(argv[i], "--verbose") == 0)
@@ -979,20 +975,16 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
 
             // Define the XML file version (and enable XML output)
             else if (std::strncmp(argv[i], "--xml-version=", 14) == 0) {
-                const std::string numberString(argv[i]+14);
-
-                std::istringstream iss(numberString);
-                if (!(iss >> mSettings.xml_version)) {
-                    printError("argument to '--xml-version' is not a number.");
+                int tmp;
+                if (!parseNumberArg(argv[i], 14, tmp))
                     return false;
-                }
-
-                if (mSettings.xml_version != 2) {
+                if (tmp != 2) {
                     // We only have xml version 2
                     printError("'--xml-version' can only be 2.");
                     return false;
                 }
 
+                mSettings.xml_version = tmp;
                 // Enable also XML if version is set
                 mSettings.xml = true;
             }

cli/cmdlineparser.h

@@ -22,6 +22,8 @@
 #include <string>
 #include <vector>
 
+#include "utils.h"
+
 class Settings;
 
 /// @addtogroup CLI
@@ -118,6 +120,24 @@ class CmdLineParser {
 private:
     bool isCppcheckPremium() const;
 
+    // TODO: get rid of is_signed
+    template<typename T>
+    static bool parseNumberArg(const char* const arg, std::size_t offset, T& num, bool is_signed = false)
+    {
+        T tmp;
+        std::string err;
+        if (!strToInt(arg + offset, tmp, &err)) {
+            printError("argument to '" + std::string(arg, offset) + "' is not valid - " + err + ".");
+            return false;
+        }
+        if (is_signed && tmp < 0) {
+            printError("argument to '" + std::string(arg, offset) + "' needs to be a positive integer.");
+            return false;
+        }
+        num = tmp;
+        return true;
+    }
+
     std::vector<std::string> mPathNames;
     std::vector<std::string> mIgnoredPaths;
     Settings &mSettings;

lib/checkbufferoverrun.cpp

@@ -143,13 +143,14 @@ static int getMinFormatStringOutputLength(const std::vector<const Token*> &param
             outputStringSize++;
 
         if (handleNextParameter) {
+            // NOLINTNEXTLINE(cert-err34-c) - intentional use
             int tempDigits = std::abs(std::atoi(digits_string.c_str()));
             if (i_d_x_f_found)
                 tempDigits = std::max(tempDigits, 1);
 
             if (digits_string.find('.') != std::string::npos) {
                 const std::string endStr = digits_string.substr(digits_string.find('.') + 1);
-                const int maxLen = std::max(std::abs(std::atoi(endStr.c_str())), 1);
+                const int maxLen = std::max(std::abs(strToInt<int>(endStr)), 1);
 
                 if (formatString[i] == 's') {
                     // For strings, the length after the dot "%.2s" will limit

lib/checkclass.cpp

@@ -3224,9 +3224,9 @@ Check::FileInfo * CheckClass::loadFileInfoFromXml(const tinyxml2::XMLElement *xm
             MyFileInfo::NameLoc nameLoc;
             nameLoc.className = name;
             nameLoc.fileName = file;
-            nameLoc.lineNumber = std::atoi(line);
-            nameLoc.column = std::atoi(col);
-            nameLoc.hash = MathLib::toULongNumber(hash);
+            nameLoc.lineNumber = strToInt<int>(line);
+            nameLoc.column = strToInt<int>(col);
+            nameLoc.hash = strToInt<std::size_t>(hash);
             fileInfo->classDefinitions.push_back(std::move(nameLoc));
         }
     }

lib/checkio.cpp

@@ -643,7 +643,7 @@ void CheckIO::checkFormatString(const Token * const tok,
                 } else if (std::isdigit(*i)) {
                     width += *i;
                 } else if (*i == '$') {
-                    parameterPosition = std::atoi(width.c_str());
+                    parameterPosition = strToInt<int>(width);
                     hasParameterPosition = true;
                     width.clear();
                 }
@@ -695,7 +695,7 @@ void CheckIO::checkFormatString(const Token * const tok,
                                 specifier += (*i == 's' || bracketBeg == formatString.end()) ? std::string{ 's' } : std::string{ bracketBeg, i + 1 };
                                 if (argInfo.variableInfo && argInfo.isKnownType() && argInfo.variableInfo->isArray() && (argInfo.variableInfo->dimensions().size() == 1) && argInfo.variableInfo->dimensions()[0].known) {
                                     if (!width.empty()) {
-                                        const int numWidth = std::atoi(width.c_str());
+                                        const int numWidth = strToInt<int>(width);
                                         if (numWidth != (argInfo.variableInfo->dimension(0) - 1))
                                             invalidScanfFormatWidthError(tok, numFormat, numWidth, argInfo.variableInfo, specifier);
                                     }
@@ -718,7 +718,7 @@ void CheckIO::checkFormatString(const Token * const tok,
                             case 'c':
                                 if (argInfo.variableInfo && argInfo.isKnownType() && argInfo.variableInfo->isArray() && (argInfo.variableInfo->dimensions().size() == 1) && argInfo.variableInfo->dimensions()[0].known) {
                                     if (!width.empty()) {
-                                        const int numWidth = std::atoi(width.c_str());
+                                        const int numWidth = strToInt<int>(width);
                                         if (numWidth > argInfo.variableInfo->dimension(0))
                                             invalidScanfFormatWidthError(tok, numFormat, numWidth, argInfo.variableInfo, std::string(1, *i));
                                     }

lib/checkunusedfunctions.cpp

@@ -442,7 +442,7 @@ void CheckUnusedFunctions::analyseWholeProgram(const Settings &settings, ErrorLo
                 } else if (std::strcmp(e2->Name(),"functiondecl") == 0) {
                     const char* lineNumber = e2->Attribute("lineNumber");
                     if (lineNumber)
-                        decls[functionName] = Location(sourcefile, std::atoi(lineNumber));
+                        decls[functionName] = Location(sourcefile, strToInt<int>(lineNumber));
                 }
             }
         }

lib/clangimport.cpp

@@ -501,19 +501,20 @@ void clangimport::AstNode::setLocations(TokenList *tokenList, int file, int line
 {
     for (const std::string &ext: mExtTokens) {
         if (ext.compare(0, 5, "<col:") == 0)
-            col = std::atoi(ext.substr(5).c_str());
+            col = strToInt<int>(ext.substr(5, ext.find_first_of(",>", 5) - 5));
         else if (ext.compare(0, 6, "<line:") == 0) {
-            line = std::atoi(ext.substr(6).c_str());
-            if (ext.find(", col:") != std::string::npos)
-                col = std::atoi(ext.c_str() + ext.find(", col:") + 6);
+            line = strToInt<int>(ext.substr(6, ext.find_first_of(":,>", 6) - 6));
+            const auto pos = ext.find(", col:");
+            if (pos != std::string::npos)
+                col = strToInt<int>(ext.substr(pos+6, ext.find_first_of(":,>", pos+6) - (pos+6)));
         } else if (ext[0] == '<') {
             const std::string::size_type colon = ext.find(':');
             if (colon != std::string::npos) {
                 const bool windowsPath = colon == 2 && ext.size() > 4 && ext[3] == '\\';
                 const std::string::size_type sep1 = windowsPath ? ext.find(':', 4) : colon;
                 const std::string::size_type sep2 = ext.find(':', sep1 + 1);
                 file = tokenList->appendFileIfNew(ext.substr(1, sep1 - 1));
-                line = MathLib::toLongNumber(ext.substr(sep1 + 1, sep2 - sep1 - 1));
+                line = strToInt<int>(ext.substr(sep1 + 1, sep2 - sep1 - 1));
             }
         }
     }
@@ -1551,7 +1552,7 @@ static void setValues(Tokenizer *tokenizer, SymbolDatabase *symbolDatabase)
             for (const Token *arrtok = tok->linkAt(1)->previous(); arrtok; arrtok = arrtok->previous()) {
                 const std::string &a = arrtok->str();
                 if (a.size() > 2 && a[0] == '[' && a.back() == ']')
-                    mul *= std::atoi(a.substr(1).c_str());
+                    mul *= strToInt<long long>(a.substr(1));
                 else
                     break;
             }