From d9deabe2ce344d4ba3f6659db2a8ee804e6b4663 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 10 Feb 2015 17:29:36 +0100 Subject: [PATCH] TestBufferOverrun: clean up --- lib/checkbufferoverrun.cpp | 99 ------------- lib/checkbufferoverrun.h | 12 -- test/cfg/posix.c | 23 +++ test/testbufferoverrun.cpp | 287 +++---------------------------------- 4 files changed, 43 insertions(+), 378 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 7c7914e03..d79325b9e 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -142,15 +142,6 @@ void CheckBufferOverrun::possibleBufferOverrunError(const Token *tok, const std: "The source buffer is larger than the destination buffer so there is the potential for overflowing the destination buffer."); } -void CheckBufferOverrun::possibleReadlinkBufferOverrunError(const Token* tok, const std::string &funcname, const std::string &varname) -{ - const std::string errmsg = funcname + "() might return the full size of '" + varname + "'. Lower the supplied size by one.\n" + - funcname + "() might return the full size of '" + varname + "'. " - "If a " + varname + "[len] = '\\0'; statement follows, it will overrun the buffer. Lower the supplied size by one."; - - reportError(tok, Severity::warning, "possibleReadlinkBufferOverrun", errmsg, true); -} - void CheckBufferOverrun::strncatUsageError(const Token *tok) { if (_settings && !_settings->isEnabled("warning")) @@ -989,11 +980,6 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo if (n > total_size) outOfBoundsError(tok->tokAt(4), "snprintf size", true, n, total_size); } - - // readlink() / readlinkat() buffer usage - if (_settings->standards.posix && Token::Match(tok, "readlink|readlinkat (")) - checkReadlinkBufferUsage(tok, scope_begin, declarationId, total_size); - } } } @@ -1021,44 +1007,6 @@ bool CheckBufferOverrun::isArrayOfStruct(const Token* tok, int &position) return false; } -void CheckBufferOverrun::checkReadlinkBufferUsage(const Token* ftok, const Token *scope_begin, const unsigned int varid, const MathLib::bigint total_size) -{ - const std::string& funcname = ftok->str(); - - const Token* bufParam = ftok->tokAt(2)->nextArgument(); - if (funcname == "readlinkat") - bufParam = bufParam ? bufParam->nextArgument() : nullptr; - if (!Token::Match(bufParam, "%varid% , %num% )", varid)) - return; - - const MathLib::bigint n = MathLib::toLongNumber(bufParam->strAt(2)); - if (total_size > 0 && n > total_size) - outOfBoundsError(bufParam, funcname + "() buf size", true, n, total_size); - - if (!_settings->inconclusive) - return; - - // only writing a part of the buffer - if (n < total_size) - return; - - // readlink()/readlinkat() never terminates the buffer, check the end of the scope for buffer termination. - bool found_termination = false; - const Token *scope_end = scope_begin->link(); - for (const Token *tok2 = bufParam->tokAt(4); tok2 && tok2 != scope_end; tok2 = tok2->next()) { - if (Token::Match(tok2, "%varid% [ %any% ] = 0 ;", bufParam->varId())) { - found_termination = true; - break; - } - } - - if (!found_termination) { - bufferNotZeroTerminatedError(ftok, bufParam->str(), funcname); - } else if (n == total_size) { - possibleReadlinkBufferOverrunError(ftok, funcname, bufParam->str()); - } -} - //--------------------------------------------------------------------------- // Checking local variables in a scope //--------------------------------------------------------------------------- @@ -1826,53 +1774,6 @@ void CheckBufferOverrun::arrayIndexThenCheckError(const Token *tok, const std::s "not be accessed if the index is out of limits."); } -// ------------------------------------------------------------------------------------- -// Check the second and the third parameter of the POSIX function write and validate -// their values. -// The parameters have the following meaning: -// - 1.parameter: file descripter (not required for this check) -// - 2.parameter: is a null terminated character string of the content to write. -// - 3.parameter: the number of bytes to write. -// -// This check is triggered if the size of the string ( 2. parameter) is lower than -// the number of bytes provided at the 3. parameter. -// -// References: -// - http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html -// - http://gd.tuwien.ac.at/languages/c/programming-bbrown/c_075.htm -// - http://codewiki.wikidot.com/c:system-calls:write -// ------------------------------------------------------------------------------------- -void CheckBufferOverrun::writeOutsideBufferSize() -{ - if (!_settings->standards.posix) - return; - - const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase(); - const std::size_t functions = symbolDatabase->functionScopes.size(); - for (std::size_t i = 0; i < functions; ++i) { - const Scope * const scope = symbolDatabase->functionScopes[i]; - for (const Token *tok = scope->classStart; tok && tok != scope->classEnd; tok = tok->next()) { - if (Token::Match(tok, "pwrite|write (") && Token::Match(tok->tokAt(2)->nextArgument(), "%str% , %num%")) { - const std::string & functionName(tok->str()); - tok = tok->tokAt(2)->nextArgument(); // set tokenptr to %str% parameter - const std::size_t stringLength = Token::getStrLength(tok)+1; // zero-terminated string! - tok = tok->tokAt(2); // set tokenptr to %num% parameter - const MathLib::bigint writeLength = MathLib::toLongNumber(tok->str()); - if (static_cast(writeLength) > stringLength) - writeOutsideBufferSizeError(tok, stringLength, writeLength, functionName); - } - } - } -} - -void CheckBufferOverrun::writeOutsideBufferSizeError(const Token *tok, const std::size_t stringLength, const MathLib::bigint writeLength, const std::string &strFunctionName) -{ - reportError(tok, Severity::error, "writeOutsideBufferSize", - "Writing " + MathLib::toString(writeLength-stringLength) + " bytes outside buffer size.\n" - "The number of bytes to write (" + MathLib::toString(writeLength) + " bytes) are bigger than the source buffer (" +MathLib::toString(stringLength)+ " bytes)." - " Please check the second and the third parameter of the function '"+strFunctionName+"'."); -} - Check::FileInfo* CheckBufferOverrun::getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const { (void)settings; diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 9f1fb06d1..96656a3b1 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -60,7 +60,6 @@ public: checkBufferOverrun.bufferOverrun(); checkBufferOverrun.bufferOverrun2(); checkBufferOverrun.arrayIndexThenCheck(); - checkBufferOverrun.writeOutsideBufferSize(); } /** @brief %Check for buffer overruns */ @@ -75,9 +74,6 @@ public: /** @brief %Check for buffer overruns by inspecting execution paths */ void executionPaths(); - /** @brief %Check using POSIX write function and writing outside buffer size */ - void writeOutsideBufferSize(); - /** * @brief Get minimum length of format string result * @param input_string format string @@ -180,9 +176,6 @@ public: /** Check for buffer overruns */ void checkScope(const Token *tok, const std::vector &varname, const ArrayInfo &arrayInfo); - /** Check readlink or readlinkat() buffer usage */ - void checkReadlinkBufferUsage(const Token *ftok, const Token *scope_begin, const unsigned int varid, const MathLib::bigint total_size); - /** * Helper function for checkFunctionCall - check a function parameter * \param tok token for the function name @@ -243,9 +236,7 @@ private: void pointerOutOfBoundsError(const Token *tok, const Token *index=nullptr, const MathLib::bigint indexvalue=0); void arrayIndexThenCheckError(const Token *tok, const std::string &indexName); void possibleBufferOverrunError(const Token *tok, const std::string &src, const std::string &dst, bool cat); - void possibleReadlinkBufferOverrunError(const Token *tok, const std::string &funcname, const std::string &varname); void argumentSizeError(const Token *tok, const std::string &functionName, const std::string &varname); - void writeOutsideBufferSizeError(const Token *tok, const std::size_t stringLength, const MathLib::bigint writeLength, const std::string& functionName); void valueFlowCheckArrayIndex(const Token * const tok, const ArrayInfo &arrayInfo); @@ -266,9 +257,7 @@ public: c.pointerOutOfBoundsError(nullptr, nullptr, 0); c.arrayIndexThenCheckError(0, "index"); c.possibleBufferOverrunError(0, "source", "destination", false); - c.possibleReadlinkBufferOverrunError(0, "readlink", "buffer"); c.argumentSizeError(0, "function", "array"); - c.writeOutsideBufferSizeError(0,2,3,"write"); c.negativeMemoryAllocationSizeError(0); c.reportError(nullptr, Severity::warning, "arrayIndexOutOfBoundsCond", "Array 'x[10]' accessed at index 20, which is out of bounds. Otherwise condition 'y==20' is redundant."); } @@ -288,7 +277,6 @@ private: "- Unsafe usage of main(argv, argc) arguments\n" "- Accessing array with index variable before checking its value\n" "- Check for large enough arrays being passed to functions\n" - "- Writing beyond bounds of a buffer\n" "- Allocating memory with a negative size\n"; } }; diff --git a/test/cfg/posix.c b/test/cfg/posix.c index acb965c7a..7aff5520a 100644 --- a/test/cfg/posix.c +++ b/test/cfg/posix.c @@ -9,6 +9,29 @@ #include +void bufferAccessOutOf(int fd) { + char a[5]; + read(fd,a,5); + // cppcheck-suppress bufferAccessOutOfBounds + read(fd,a,6); + write(fd,a,5); + // cppcheck-suppress bufferAccessOutOfBounds + write(fd,a,6); + recv(fd,a,5,0); + // cppcheck-suppress bufferAccessOutOfBounds + recv(fd,a,6,0); + recvfrom(fd,a,5,0,0x0,0x0); + // cppcheck-suppress bufferAccessOutOfBounds + recvfrom(fd,a,6,0,0x0,0x0); + send(fd,a,5,0); + // cppcheck-suppress bufferAccessOutOfBounds + send(fd,a,6,0); + sendto(fd,a,5,0,0x0,0x0); + // cppcheck-suppress bufferAccessOutOfBounds + sendto(fd,a,6,0,0x0,0x0); + 0; +} + void f(char *p) { isatty (0); mkdir (p, 0); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index d4485c808..7ed4cd072 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -40,7 +40,6 @@ private: Settings settings; settings.inconclusive = true; - settings.standards.posix = true; settings.experimental = experimental; settings.addEnabled("warning"); settings.addEnabled("style"); @@ -68,7 +67,6 @@ private: checkBufferOverrun.bufferOverrun(); checkBufferOverrun.bufferOverrun2(); checkBufferOverrun.arrayIndexThenCheck(); - checkBufferOverrun.writeOutsideBufferSize(); } void check(const char code[], const Settings &settings, const char filename[] = "test.cpp") { @@ -85,7 +83,6 @@ private: checkBufferOverrun.bufferOverrun(); checkBufferOverrun.bufferOverrun2(); checkBufferOverrun.arrayIndexThenCheck(); - checkBufferOverrun.writeOutsideBufferSize(); } void checkstd(const char code[], const char filename[] = "test.cpp") { @@ -99,31 +96,6 @@ private: check(code, settings, filename); } - void checkposix(const char code[], const char filename[] = "test.cpp") { - static bool init; - static Settings settings; - if (!init) { - init = true; - LOAD_LIB_2(settings.library, "posix.cfg"); - settings.addEnabled("warning"); - } - - Tokenizer tokenizer(&settings, this); - std::istringstream istr(code); - tokenizer.tokenize(istr, filename); - - // Clear the error buffer.. - errout.str(""); - - // Check for buffer overruns.. - CheckBufferOverrun checkBufferOverrun(&tokenizer, &settings, this); - checkBufferOverrun.bufferOverrun(); - checkBufferOverrun.bufferOverrun2(); - checkBufferOverrun.arrayIndexThenCheck(); - checkBufferOverrun.writeOutsideBufferSize(); - } - - void run() { TEST_CASE(noerr1); TEST_CASE(noerr2); @@ -197,8 +169,6 @@ private: TEST_CASE(array_index_valueflow); TEST_CASE(array_index_function_parameter); - TEST_CASE(buffer_overrun_1_standard_functions); - TEST_CASE(buffer_overrun_1_posix_functions); TEST_CASE(buffer_overrun_2_struct); TEST_CASE(buffer_overrun_3); TEST_CASE(buffer_overrun_4); @@ -318,10 +288,6 @@ private: TEST_CASE(arrayIndexThenCheck); TEST_CASE(bufferNotZeroTerminated); - TEST_CASE(readlink); - TEST_CASE(readlinkat); - - TEST_CASE(writeOutsideBufferSize) TEST_CASE(negativeMemoryAllocationSizeError) // #389 @@ -2159,95 +2125,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void buffer_overrun_1_posix_functions() { - checkposix("void f(int fd)\n" - "{\n" - " char str[3];\n" - " read(fd, str, 3);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - checkposix("void f(int fd)\n" - "{\n" - " char str[3];\n" - " read(fd, str, 4);\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: str\n", errout.str()); - - checkposix("void f(int fd)\n" - "{\n" - " char str[3];\n" - " write(fd, str, 3);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - checkposix("void f(int fd)\n" - "{\n" - " char str[3];\n" - " write(fd, str, 4);\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: str\n", errout.str()); - - checkposix("void f()\n" - "{\n" - " long bb[2];\n" - " write(stdin, bb, sizeof(bb));\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - checkposix("void f()\n" - "{\n" - "char str[3];\n" - "recv(s, str, 4, 0);\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: str\n", errout.str()); - - checkposix("void f()\n" - "{\n" - "char str[3];\n" - "recvfrom(s, str, 4, 0, 0x0, 0x0);\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: str\n", errout.str()); - - checkposix("void f()\n" - "{\n" - "char str[3];\n" - "send(s, str, 4, 0);\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: str\n", errout.str()); - - checkposix("void f()\n" - "{\n" - "char str[3];\n" - "sendto(s, str, 4, 0, 0x0, 0x0);\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: str\n", errout.str()); - } - - void buffer_overrun_1_standard_functions() { - - // #4968 - not standard function - checkstd("void f() {\n" - " char str[3];\n" - " foo.memset(str, 0, 100);\n" - " foo::memset(str, 0, 100);\n" - " std::memset(str, 0, 100);\n" - "}"); - TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: str\n", "", errout.str()); - - // #5257 - check strings - checkstd("void f() {\n" - " memcpy(temp, \"hello world\", 20);\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (error) Buffer is accessed out of bounds.\n", errout.str()); - - checkstd("void f() {\n" - " memcpy(temp, \"abc\", 4);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void buffer_overrun_2_struct() { check("struct ABC\n" "{\n" @@ -3713,6 +3590,26 @@ private: " memset(a+5, 0, 10);\n" "}", settings); ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds: a\n", errout.str()); + + // #4968 - not standard function + check("void f() {\n" + " char str[3];\n" + " foo.memset(str, 0, 100);\n" + " foo::memset(str, 0, 100);\n" + " std::memset(str, 0, 100);\n" + "}", settings); + TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: str\n", "", errout.str()); + + // #5257 - check strings + check("void f() {\n" + " memset(\"abc\", 0, 20);\n" + "}", settings); + ASSERT_EQUALS("[test.cpp:2]: (error) Buffer is accessed out of bounds.\n", errout.str()); + + check("void f() {\n" + " memset(temp, \"abc\", 4);\n" + "}", settings); + ASSERT_EQUALS("", errout.str()); } void minsize_sizeof() { @@ -4233,150 +4130,6 @@ private: ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) The buffer 'c' is not null-terminated after the call to memmove().\n", errout.str()); } - void readlink() { - check("void f() {\n" - " char buf[255];\n" - " ssize_t len = readlink(path, buf, 254);\n" - " printf(\"%s\n\", buf);\n" - "}"); - TODO_ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) The buffer 'buf' is not null-terminated after the call to readlink().\n", "", errout.str()); - - // C only: Primitive pointer simplification - check("void f() {\n" - " char buf[255];\n" - " ssize_t len = readlink(path, &buf[0], 254);\n" - " printf(\"%s\n\", buf);\n" - "}\n", true, "test.c"); - TODO_ASSERT_EQUALS("[test.c:3]: (warning, inconclusive) The buffer 'buf' is not null-terminated after the call to readlink().\n", "", errout.str()); - - check("void f() {\n" - " char buf[255];\n" - " ssize_t len = readlink(path, buf, 254);\n" - " buf[len] = 0;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" - " char buf[10];\n" - " ssize_t len = readlink(path, buf, 255);\n" - " buf[len] = 0;\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) readlink() buf size is out of bounds: Supplied size 255 is larger than actual size 10.\n", errout.str()); - - check("void f() {\n" - " char buf[255];\n" - " ssize_t len = readlink(path, buf, 255);\n" - " buf[len] = 0;\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) readlink() might return the full size of 'buf'. Lower the supplied size by one.\n", errout.str()); - - check("void f() {\n" - " char buf[255];\n" - " ssize_t len = readlink(path, buf, 254);\n" - " if (len == -1) {\n" - " return;\n" - " }\n" - " buf[len] = 0;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" - " char buf[255] = {0};\n" - " readlink(path, buf, 254);\n" // <- doesn't write whole buf - " puts(buf);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void readlinkat() { - check("void f() {\n" - " char buf[255];\n" - " ssize_t len = readlinkat(42, path, buf, 254);\n" - " printf(\"%s\n\", buf);\n" - "}"); - TODO_ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) The buffer 'buf' is not null-terminated after the call to readlinkat().\n", "", errout.str()); - - check("void f() {\n" - " char buf[255];\n" - " ssize_t len = readlinkat(42, path, buf, 254);\n" - " buf[len] = 0;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" - " char buf[10];\n" - " ssize_t len = readlinkat(42, path, buf, 255);\n" - " buf[len] = 0;\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) readlinkat() buf size is out of bounds: Supplied size 255 is larger than actual size 10.\n", errout.str()); - - check("void f() {\n" - " char buf[255];\n" - " ssize_t len = readlinkat(42, path, buf, 255);\n" - " buf[len] = 0;\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) readlinkat() might return the full size of 'buf'. Lower the supplied size by one.\n", errout.str()); - - check("void f() {\n" - " char buf[255];\n" - " ssize_t len = readlinkat(42, path, buf, 254);\n" - " if (len == -1) {\n" - " return;\n" - " }\n" - " buf[len] = 0;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void writeOutsideBufferSize() { - check("void f(void){\n" - "write(1, \"Dump string \\n\", 100);\n" - "}"); // ^ number of bytes too big - ASSERT_EQUALS("[test.cpp:2]: (error) Writing 86 bytes outside buffer size.\n", errout.str()); - - check("void f(void){\n" - "write(1, \"Dump string \\n\", 10);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // #4706 avoid crashing when a struct member is used as first argument - check("static struct {\n" - " int i[2];\n" - "} p;\n" - "void foo()\n" - "{\n" - " write(p.i[1], \"\", 1);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("static struct {\n" - " int i[2];\n" - "} p;\n" - "void foo()\n" - "{\n" - " write(p.i[1], \"\", 2);\n" - "}"); - ASSERT_EQUALS("[test.cpp:6]: (error) Writing 1 bytes outside buffer size.\n", errout.str()); - // #4969 - check("void foo()\n" - "{\n" - " write(1, \"\\0\", 1);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - // that is documented to be ok - check("void foo()\n" - "{\n" - " write(1, 0, 0);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - // ... that is not ok - check("void foo()\n" - "{\n" - " write(1, 0, 1);\n" - "}"); - TODO_ASSERT_EQUALS("[test.cpp:3]: (error) Writing 1 bytes outside buffer size.\n", "", errout.str()); - } - void negativeMemoryAllocationSizeError() { // #389 check("void f()\n" "{\n"