From a1b0d190df894d0e9eff7ae84cd063d95c8d77f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 2 Jan 2014 10:46:19 +0100 Subject: [PATCH] Fixed #3688 (false positive: (inconclusive, posix) (warning) The buffer 'cBuffer' is not zero-terminated after the call to readlink().) --- lib/checkbufferoverrun.cpp | 29 ++++++++++++++++------------- lib/checkbufferoverrun.h | 2 +- test/testbufferoverrun.cpp | 13 ++++++++++--- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index efa2b2496..a264536f3 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -1304,12 +1304,8 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo } // readlink() / readlinkat() buffer usage - if (_settings->standards.posix) { - if (Token::simpleMatch(tok, "readlink (") && Token::Match(tok->tokAt(2)->nextArgument(), "%varid% , %num% )", arrayInfo.declarationId())) - checkReadlinkBufferUsage(tok, scope_begin, total_size, false); - else if (Token::simpleMatch(tok, "readlinkat (") && Token::Match(tok->tokAt(2)->nextArgument()->nextArgument(), "%varid% , %num% )", arrayInfo.declarationId())) - checkReadlinkBufferUsage(tok, scope_begin, total_size, true); - } + if (_settings->standards.posix && Token::Match(tok, "readlink|readlinkat (")) + checkReadlinkBufferUsage(tok, scope_begin, arrayInfo.declarationId(), total_size); // undefined behaviour: result of pointer arithmetic is out of bounds if (_settings->isEnabled("portability") && Token::Match(tok, "= %varid% + %num% ;", arrayInfo.declarationId())) { @@ -1344,12 +1340,15 @@ bool CheckBufferOverrun::isArrayOfStruct(const Token* tok, int &position) return false; } -void CheckBufferOverrun::checkReadlinkBufferUsage(const Token* tok, const Token *scope_begin, const MathLib::bigint total_size, const bool is_readlinkat) +void CheckBufferOverrun::checkReadlinkBufferUsage(const Token* ftok, const Token *scope_begin, const unsigned int varid, const MathLib::bigint total_size) { - const Token* bufParam = tok->tokAt(2)->nextArgument(); - if (is_readlinkat) - bufParam = bufParam->nextArgument(); - const std::string funcname = is_readlinkat ? "readlinkat" : "readlink"; + const std::string funcname = ftok->str(); + + const Token* bufParam = ftok->tokAt(2)->nextArgument(); + if (funcname == "readlinkat") + bufParam = bufParam ? bufParam->nextArgument() : NULL; + if (!Token::Match(bufParam, "%varid% , %num% )", varid)) + return; const MathLib::bigint n = MathLib::toLongNumber(bufParam->strAt(2)); if (total_size > 0 && n > total_size) @@ -1358,6 +1357,10 @@ void CheckBufferOverrun::checkReadlinkBufferUsage(const Token* tok, const Token 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(); @@ -1369,9 +1372,9 @@ void CheckBufferOverrun::checkReadlinkBufferUsage(const Token* tok, const Token } if (!found_termination) { - bufferNotZeroTerminatedError(tok, bufParam->str(), funcname); + bufferNotZeroTerminatedError(ftok, bufParam->str(), funcname); } else if (n == total_size) { - possibleReadlinkBufferOverrunError(tok, funcname, bufParam->str()); + possibleReadlinkBufferOverrunError(ftok, funcname, bufParam->str()); } } diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 91c3fb3f5..22fd84e01 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -191,7 +191,7 @@ public: void parse_for_body(const Token *tok2, const ArrayInfo &arrayInfo, const std::string &strindex, bool condition_out_of_bounds, unsigned int counter_varid, const std::string &min_counter_value, const std::string &max_counter_value); /** Check readlink or readlinkat() buffer usage */ - void checkReadlinkBufferUsage(const Token *tok, const Token *scope_begin, const MathLib::bigint total_size, const bool is_readlinkat); + 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 diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index c47ab9d59..56a5b56ef 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -3940,7 +3940,7 @@ private: " ssize_t len = readlink(path, buf, 254);\n" " printf(\"%s\n\", buf);\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) The buffer 'buf' is not null-terminated after the call to readlink().\n", errout.str()); + 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" @@ -3948,7 +3948,7 @@ private: " ssize_t len = readlink(path, &buf[0], 254);\n" " printf(\"%s\n\", buf);\n" "}\n", true, "test.c"); - ASSERT_EQUALS("[test.c:3]: (warning, inconclusive) The buffer 'buf' is not null-terminated after the call to readlink().\n", errout.str()); + 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" @@ -3980,6 +3980,13 @@ private: " 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() { @@ -3988,7 +3995,7 @@ private: " ssize_t len = readlinkat(42, path, buf, 254);\n" " printf(\"%s\n\", buf);\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) The buffer 'buf' is not null-terminated after the call to readlinkat().\n", errout.str()); + 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"