From 55d9f0873a3e60b64e24778e6efab79f34f98177 Mon Sep 17 00:00:00 2001 From: Thomas Jarosch Date: Sat, 22 Oct 2011 11:54:52 +0200 Subject: [PATCH] Fix #3243 (Improve non reentrant function check) - Use std::map instead of linear std::list walk and run fast tests like tok->isName() first. Global speed up is 4.8% (profiled with google-perftools) - Catch function invocations in global namespace and ignore other namespaces except "std". std::localtime() and others are also non-thread safe on POSIX. Note: The check matches f.e. also on "std::getrpcbyname()", but that would result in a compile error anyway. No need to have an extra "std::xxxxx" whitelist. - Remove double listed "rand" and "getrpcbyname" function names --- lib/checknonreentrantfunctions.cpp | 31 ++++++++++++----- lib/checknonreentrantfunctions.h | 14 ++++---- test/testnonreentrantfunctions.cpp | 54 ++++++++++++++++++++++++++++-- 3 files changed, 82 insertions(+), 17 deletions(-) diff --git a/lib/checknonreentrantfunctions.cpp b/lib/checknonreentrantfunctions.cpp index 0066a9d94..0f4aecec4 100644 --- a/lib/checknonreentrantfunctions.cpp +++ b/lib/checknonreentrantfunctions.cpp @@ -39,16 +39,31 @@ void CheckNonReentrantFunctions::nonReentrantFunctions() if (_tokenizer->isJavaOrCSharp()) return; + std::map::const_iterator nonReentrant_end = _nonReentrantFunctions.end(); for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - std::list< std::pair >::const_iterator it(_nonReentrantFunctions.begin()), itend(_nonReentrantFunctions.end()); - for (; it!=itend; ++it) { - if (tok->strAt(1) == it->first && tok->strAt(2) == "(" && tok->tokAt(1)->varId() == 0 && !tok->tokAt(0)->isName() && !Token::Match(tok, ".|::|:|,")) { - // If checking code that is single threaded, this might be not interesing for all. - // Therefore this is "portabiblity" - reportError(tok->tokAt(1), Severity::portability, "nonreentrantFunctions"+it->first, it->second); - break; - } + // Look for function invocations + if (!tok->isName() || tok->strAt(1) != "(" || tok->varId() != 0) + continue; + + // Check for non-reentrant function name + std::map::const_iterator it = _nonReentrantFunctions.find(tok->str()); + if (it == nonReentrant_end) + continue; + + const Token *prev = tok->previous(); + if (prev) + { + // Ignore function definitions, class members or class definitions + if (prev->isName() || Token::Match(prev, ".|:")) + continue; + + // Check for "std" or global namespace, ignore other namespaces + if (prev->str() == "::" && prev->previous() && prev->previous()->str() != "std" && prev->previous()->isName()) + continue; } + + // Only affecting multi threaded code, therefore this is "portability" + reportError(tok, Severity::portability, "nonreentrantFunctions"+it->first, it->second); } } //--------------------------------------------------------------------------- diff --git a/lib/checknonreentrantfunctions.h b/lib/checknonreentrantfunctions.h index 74e20a4bd..5e14b0290 100644 --- a/lib/checknonreentrantfunctions.h +++ b/lib/checknonreentrantfunctions.h @@ -24,7 +24,7 @@ #include "check.h" #include -#include +#include /// @addtogroup Checks @@ -58,7 +58,7 @@ public: private: /* function name / error message */ - std::list< std::pair< const std::string, const std::string> > _nonReentrantFunctions; + std::map _nonReentrantFunctions; /** init nonreentrant functions list ' */ void initNonReentrantFunctions() { @@ -66,8 +66,8 @@ private: "ctime", "localtime", "gmtime", "asctime", "strtok", "gethostbyname", "gethostbyaddr", "getservbyname" , "getservbyport", "crypt", "ttyname", "rand", "gethostbyname2" , "getprotobyname", "getnetbyname", "getnetbyaddr", "getrpcbyname", "getrpcbynumber", "getrpcent" - , "rand", "ctermid", "tmpnam", "readdir", "getlogin", "getpwent", "getpwnam", "getpwuid", "getspent" - , "fgetspent", "getspnam", "getgrnam", "getgrgid", "getnetgrent", "getrpcbyname", "tempnam", "fgetpwent" + , "ctermid", "tmpnam", "readdir", "getlogin", "getpwent", "getpwnam", "getpwuid", "getspent" + , "fgetspent", "getspnam", "getgrnam", "getgrgid", "getnetgrent", "tempnam", "fgetpwent" , "fgetgrent", "ecvt", "gcvt", "getservent", "gethostent", "getgrent", "fcvt" }; @@ -78,14 +78,14 @@ private: strMsg+= "\'. For threadsafe applications it is recommended to use the reentrant replacement function \'"; strMsg+=non_reentrant_functions_list[i]; strMsg+="_r\'"; - _nonReentrantFunctions.push_back(std::make_pair(non_reentrant_functions_list[i],strMsg)); + _nonReentrantFunctions[non_reentrant_functions_list[i]] = strMsg; } } void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) { CheckNonReentrantFunctions c(0, settings, errorLogger); - std::list< std::pair >::const_iterator it(_nonReentrantFunctions.begin()), itend(_nonReentrantFunctions.end()); + std::map::const_iterator it(_nonReentrantFunctions.begin()), itend(_nonReentrantFunctions.end()); for (; it!=itend; ++it) { c.reportError(0, Severity::portability, "nonreentrantFunctions"+it->first, it->second); } @@ -97,7 +97,7 @@ private: std::string classInfo() const { std::string info = "Warn if any of these non reentrant functions are used:\n"; - std::list< std::pair >::const_iterator it(_nonReentrantFunctions.begin()), itend(_nonReentrantFunctions.end()); + std::map::const_iterator it(_nonReentrantFunctions.begin()), itend(_nonReentrantFunctions.end()); for (; it!=itend; ++it) { info += "* " + it->first + "\n"; } diff --git a/test/testnonreentrantfunctions.cpp b/test/testnonreentrantfunctions.cpp index b0730cf7e..55b3be191 100644 --- a/test/testnonreentrantfunctions.cpp +++ b/test/testnonreentrantfunctions.cpp @@ -34,6 +34,7 @@ private: void run() { TEST_CASE(test_crypt); + TEST_CASE(test_namespace_handling); } void check(const char code[]) { @@ -85,8 +86,57 @@ private: ASSERT_EQUALS("", errout.str()); } + void test_namespace_handling() { + check("int f()\n" + "{\n" + " time_t t = 0;" + " std::localtime(&t);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (portability) Found non reentrant function 'localtime'. For threadsafe applications it is recommended to use the reentrant replacement function 'localtime_r'\n", errout.str()); + + // Passed as function argument + check("int f()\n" + "{\n" + " printf(\"Magic guess: %d\n\", rand());\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (portability) Found non reentrant function 'rand'. For threadsafe applications it is recommended to use the reentrant replacement function 'rand_r'\n", errout.str()); + + // Pass return value + check("int f()\n" + "{\n" + " time_t t = 0;" + " struct tm *foo = localtime(&t);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (portability) Found non reentrant function 'localtime'. For threadsafe applications it is recommended to use the reentrant replacement function 'localtime_r'\n", errout.str()); + + // Access via global namespace + check("int f()\n" + "{\n" + " ::rand();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (portability) Found non reentrant function 'rand'. For threadsafe applications it is recommended to use the reentrant replacement function 'rand_r'\n", errout.str()); + + // Be quiet on function definitions + check("int rand()\n" + "{\n" + " return 123;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + // Be quiet on other namespaces + check("int f()\n" + "{\n" + " foobar::rand();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + // Be quiet on class member functions + check("int f()\n" + "{\n" + " foobar.rand();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestNonReentrantFunctions) - -