diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index ce93d75c8..d46f9e5aa 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -107,6 +107,15 @@ 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 &varname) +{ + const std::string errmsg = "readlink() might return the full size of '" + varname + "'. Lower the supplied size by one.\n" + "readlink() might return the full size of '" + varname + "'. Lower the supplied size by one. " + "If a " + varname + "[len] = '\\0'; statement follows, it will overrun the buffer."; + + reportInconclusiveError(tok, Severity::warning, "possibleReadlinkBufferOverrun", errmsg); +} + void CheckBufferOverrun::strncatUsageError(const Token *tok) { if (_settings && !_settings->isEnabled("style")) @@ -160,9 +169,11 @@ void CheckBufferOverrun::cmdLineArgsError(const Token *tok) void CheckBufferOverrun::bufferNotZeroTerminatedError(const Token *tok, const std::string &varname, const std::string &function) { - reportError(tok, Severity::warning, "bufferNotZeroTerminated", - "The buffer '" + varname + "' is not zero-terminated after the call to " + function + "().\n" - "This will cause bugs later in the code if the code assumes buffer is zero-terminated."); + const std::string errmsg = "The buffer '" + varname + "' is not zero-terminated after the call to " + function + "().\n" + "The buffer '" + varname + "' is not zero-terminated after the call to " + function + "(). " + "This will cause bugs later in the code if the code assumes the buffer is zero-terminated."; + + reportInconclusiveError(tok, Severity::warning, "bufferNotZeroTerminated", errmsg); } //--------------------------------------------------------------------------- @@ -993,6 +1004,9 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo { const MathLib::bigint total_size = arrayInfo.num(0) * arrayInfo.element_size(); + const Token *scope_begin = tok->previous(); + assert(scope_begin != 0); + // Count { and } for tok unsigned int indentlevel = 0; for (; tok; tok = tok->next()) { @@ -1178,6 +1192,31 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo outOfBoundsError(tok->tokAt(4), "snprintf size", true, n, total_size); } + // readlink() + if (_settings->posix && Token::Match(tok, "readlink ( %any% , %varid% , %num% )", arrayInfo.varid())) { + const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(6)); + if (total_size > 0 && n > total_size) + outOfBoundsError(tok->tokAt(4), "readlink() buf size", true, n, total_size); + + if (_settings->inconclusive) { + // readlink() 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 = tok->tokAt(8); tok2 && tok2 != scope_end; tok2 = tok2->next()) { + if (Token::Match(tok2, "%varid% [ %any% ] = 0 ;", tok->tokAt(4)->varId())) { + found_termination = true; + break; + } + } + + if (!found_termination) { + bufferNotZeroTerminatedError(tok, tok->tokAt(4)->str(), "readlink"); + } else if (n == total_size) { + possibleReadlinkBufferOverrunError(tok, tok->tokAt(4)->str()); + } + } + } + // undefined behaviour: result of pointer arithmetic is out of bounds if (_settings->isEnabled("portability") && Token::Match(tok, "= %varid% + %num% ;", arrayInfo.varid())) { const MathLib::bigint index = MathLib::toLongNumber(tok->strAt(3)); diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index b841f5047..93c2e39c6 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -223,6 +223,7 @@ public: void pointerOutOfBoundsError(const Token *tok, const std::string &object); // UB when result of calculation is out of bounds 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 &varname); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) { CheckBufferOverrun c(0, settings, errorLogger); @@ -240,6 +241,7 @@ public: c.pointerOutOfBoundsError(0, "array"); c.arrayIndexThenCheckError(0, "index"); c.possibleBufferOverrunError(0, "source", "destination", false); + c.possibleReadlinkBufferOverrunError(0, "buffer"); } std::string myName() const { diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 6cbcc81d5..40823a370 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -41,6 +41,7 @@ private: Settings settings; settings.inconclusive = true; + settings.posix = true; settings.experimental = experimental; settings.addEnabled("style"); settings.addEnabled("portability"); @@ -234,6 +235,7 @@ private: TEST_CASE(arrayIndexThenCheck); TEST_CASE(bufferNotZeroTerminated); + TEST_CASE(readlink); } @@ -3424,8 +3426,51 @@ private: "}\n"); ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'c' is not zero-terminated after the call to memmove().\n", errout.str()); } + + void readlink() { + check("void f()\n" + "{\n" + " char buf[255];\n" + " ssize_t len = readlink(path, buf, sizeof(buf)-1);\n" + " printf(\"%s\n\", buf);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'buf' is not zero-terminated after the call to readlink().\n", errout.str()); + + check("void f()\n" + "{\n" + " char buf[255];\n" + " ssize_t len = readlink(path, buf, sizeof(buf)-1);\n" + " buf[len] = 0;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f()\n" + "{\n" + " char buf[10];\n" + " ssize_t len = readlink(path, buf, 255);\n" + " buf[len] = 0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) readlink() buf size is out of bounds: Supplied size 255 is larger than actual size of 10\n", errout.str()); + + check("void f()\n" + "{\n" + " char buf[255];\n" + " ssize_t len = readlink(path, buf, sizeof(buf));\n" + " buf[len] = 0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (warning) readlink() might return the full size of 'buf'. Lower the supplied size by one.\n", errout.str()); + + check("void f()\n" + "{\n" + " char buf[255];\n" + " ssize_t len = readlink(path, buf, sizeof(buf)-1);\n" + " if (len == -1) {\n" + " return;\n" + " }\n" + " buf[len] = 0;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestBufferOverrun) - -