From bc594d52c8f678bb136f562095c1da0f17425a7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 25 Dec 2014 10:05:55 +0100 Subject: [PATCH] Fixed #6349 (Pointer arithmetic: clarify message) --- lib/checkbufferoverrun.cpp | 41 ++++++++++++++++++++++++-------------- lib/checkbufferoverrun.h | 4 ++-- test/testbufferoverrun.cpp | 8 ++++---- 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 49799ecce..61040742f 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -173,13 +173,24 @@ void CheckBufferOverrun::outOfBoundsError(const Token *tok, const std::string &w reportError(tok, Severity::error, "outOfBounds", oss.str()); } -void CheckBufferOverrun::pointerOutOfBoundsError(const Token *tok, const std::string &object) +void CheckBufferOverrun::pointerOutOfBoundsError(const Token *tok, const Token *index, const MathLib::bigint indexvalue) { // The severity is portability instead of error since this ub doesnt // cause bad behaviour on most implementations. people create out // of bounds pointers by intention. - reportError(tok, Severity::portability, "pointerOutOfBounds", "Undefined behaviour: Pointer arithmetic result does not point into or just past the end of the " + object + ".\n" - "Undefined behaviour: The result of this pointer arithmetic does not point into or just one element past the end of the " + object + ". Further information: https://www.securecoding.cert.org/confluence/display/seccode/ARR30-C.+Do+not+form+or+use+out+of+bounds+pointers+or+array+subscripts"); + const std::string expr(tok ? tok->expressionString() : std::string("")); + std::string errmsg("Undefined behaviour. Pointer arithmetic '" + expr + "' result is out of bounds"); + if (index && !index->isNumber()) + errmsg += " when " + index->expressionString() + " is " + MathLib::toString(indexvalue); + std::string verbosemsg(errmsg + ". From chapter 6.5.6 in the C specification:\n" + "\"When an expression that has integer type is added to or subtracted from a pointer, ..\" and then \"If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined.\""); + reportError(tok, Severity::portability, "pointerOutOfBounds", errmsg + ".\n" + verbosemsg); + /* + "Undefined behaviour: The result of this pointer arithmetic does not point into + or just one element past the end of the " + object + ". + Further information: + https://www.securecoding.cert.org/confluence/display/seccode/ARR30-C.+Do+not+form+or+use+out+of+bounds+pointers+or+array+subscripts"); + */ } void CheckBufferOverrun::sizeArgumentAsCharError(const Token *tok) @@ -699,7 +710,7 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vectorstrAt(3)); if (isPortabilityEnabled && index > size) - pointerOutOfBoundsError(tok->next(), "buffer"); + pointerOutOfBoundsError(tok->tokAt(2)); if (index >= size && Token::Match(tok->tokAt(-2), "[;{}] %varid% =", declarationId)) pointerIsOutOfBounds = true; } @@ -851,24 +862,24 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo } else if (isPortabilityEnabled && !tok->isCast() && tok->astParent() && tok->astParent()->str() == "+") { - const ValueFlow::Value *index; - if (tok == tok->astParent()->astOperand1()) - index = tok->astParent()->astOperand2()->getMaxValue(false); - else - index = tok->astParent()->astOperand1()->getMaxValue(false); - // undefined behaviour: result of pointer arithmetic is out of bounds - if (index && (index->intvalue < 0 || index->intvalue > arrayInfo.num(0))) { - pointerOutOfBoundsError(tok, "array"); - } + const Token *index; + if (tok == tok->astParent()->astOperand1()) + index = tok->astParent()->astOperand2(); + else + index = tok->astParent()->astOperand1(); + const ValueFlow::Value *value = index ? index->getMaxValue(false) : nullptr; + if (value && (value->intvalue < 0 || value->intvalue > arrayInfo.num(0))) + pointerOutOfBoundsError(tok->astParent(), index, value->intvalue); } else if (isPortabilityEnabled && tok->astParent() && tok->astParent()->str() == "-") { const Variable *var = _tokenizer->getSymbolDatabase()->getVariableFromVarId(declarationId); if (var && var->isArray()) { const Token *index = tok->astParent()->astOperand2(); - if (index && index->getValueGE(1,_settings)) - pointerOutOfBoundsError(tok, "array"); + const ValueFlow::Value *value = index ? index->getValueGE(1,_settings) : nullptr; + if (value) + pointerOutOfBoundsError(tok->astParent(), index, value->intvalue); } } } diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index cdd79a322..db3efed0d 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -240,7 +240,7 @@ private: void negativeIndexError(const Token *tok, MathLib::bigint index); void negativeIndexError(const Token *tok, const ValueFlow::Value &index); void cmdLineArgsError(const Token *tok); - void pointerOutOfBoundsError(const Token *tok, const std::string &object); // UB when result of calculation is out of bounds + 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); @@ -263,7 +263,7 @@ public: c.bufferNotZeroTerminatedError(0, "buffer", "strncpy"); c.negativeIndexError(0, -1); c.cmdLineArgsError(0); - c.pointerOutOfBoundsError(0, "array"); + c.pointerOutOfBoundsError(nullptr, nullptr, 0); c.arrayIndexThenCheckError(0, "index"); c.possibleBufferOverrunError(0, "source", "destination", false); c.possibleReadlinkBufferOverrunError(0, "readlink", "buffer"); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 7c96dd724..2875ff7f1 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -2962,13 +2962,13 @@ private: " char a[10];\n" " char *p = a + 100;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (portability) Undefined behaviour: Pointer arithmetic result does not point into or just past the end of the array.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (portability) Undefined behaviour. Pointer arithmetic 'a+100' result is out of bounds.\n", errout.str()); check("void f() {\n" " char a[10];\n" " return a + 100;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (portability) Undefined behaviour: Pointer arithmetic result does not point into or just past the end of the array.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (portability) Undefined behaviour. Pointer arithmetic 'a+100' result is out of bounds.\n", errout.str()); check("void f() {\n" // #6350 - fp when there is cast of buffer " wchar_t buf[64];\n" @@ -2983,7 +2983,7 @@ private: " p += 100;\n" " free(p);" "}"); - ASSERT_EQUALS("[test.cpp:3]: (portability) Undefined behaviour: Pointer arithmetic result does not point into or just past the end of the buffer.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (portability) Undefined behaviour. Pointer arithmetic 'p+100' result is out of bounds.\n", errout.str()); check("void f() {\n" " char *p = malloc(10);\n" @@ -3017,7 +3017,7 @@ private: " char x[10];\n" " return x-1;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (portability) Undefined behaviour: Pointer arithmetic result does not point into or just past the end of the array.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (portability) Undefined behaviour. Pointer arithmetic 'x-1' result is out of bounds.\n", errout.str()); } void sprintf1() {