diff --git a/cfg/cppcheck-cfg.rng b/cfg/cppcheck-cfg.rng index 4f3758b8e..595afb2be 100644 --- a/cfg/cppcheck-cfg.rng +++ b/cfg/cppcheck-cfg.rng @@ -165,6 +165,20 @@ + + + + + + + + + + + + + + diff --git a/cfg/std.cfg b/cfg/std.cfg index b65039679..6882548c7 100644 --- a/cfg/std.cfg +++ b/cfg/std.cfg @@ -3863,6 +3863,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun false + diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 14b64fe70..f41109da3 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3358,3 +3358,116 @@ void CheckOther::checkModuloOfOneError(const Token *tok) { reportError(tok, Severity::style, "moduloofone", "Modulo of one is always equal to zero"); } + +//----------------------------------------------------------------------------- +// Overlapping write (undefined behavior) +//----------------------------------------------------------------------------- +static bool getBufAndOffset(const Token *expr, const Token **buf, MathLib::bigint *offset) +{ + if (!expr) + return false; + const Token *bufToken, *offsetToken; + if (expr->isUnaryOp("&") && Token::simpleMatch(expr->astOperand1(), "[")) { + bufToken = expr->astOperand1()->astOperand1(); + offsetToken = expr->astOperand1()->astOperand2(); + } else if (Token::Match(expr, "+|-") && expr->isBinaryOp()) { + const bool pointer1 = (expr->astOperand1()->valueType() && expr->astOperand1()->valueType()->pointer > 0); + const bool pointer2 = (expr->astOperand2()->valueType() && expr->astOperand2()->valueType()->pointer > 0); + if (pointer1 && !pointer2) { + bufToken = expr->astOperand1(); + offsetToken = expr->astOperand2(); + } else if (!pointer1 && pointer2) { + bufToken = expr->astOperand2(); + offsetToken = expr->astOperand1(); + } else { + return false; + } + } else if (expr->valueType() && expr->valueType()->pointer > 0) { + *buf = expr; + *offset = 0; + return true; + } else { + return false; + } + if (!bufToken->valueType() || !bufToken->valueType()->pointer) + return false; + if (!offsetToken->hasKnownIntValue()) + return false; + *buf = bufToken; + *offset = offsetToken->getKnownIntValue(); + return true; +} + +void CheckOther::checkOverlappingWrite() +{ + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope *functionScope : symbolDatabase->functionScopes) { + for (const Token *tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { + if (tok->isAssignmentOp()) { + // check if LHS is a union member.. + const Token * const lhs = tok->astOperand1(); + if (!Token::simpleMatch(lhs, ".") || !lhs->isBinaryOp()) + continue; + const Variable * const lhsvar = lhs->astOperand1()->variable(); + if (!lhsvar || !lhsvar->typeScope() || lhsvar->typeScope()->type != Scope::ScopeType::eUnion) + continue; + const Token* const lhsmember = lhs->astOperand2(); + if (!lhsmember) + continue; + + // Is other union member used in RHS? + const Token *errorToken = nullptr; + visitAstNodes(tok->astOperand2(), [lhsvar, lhsmember, &errorToken](const Token *rhs) { + if (!Token::simpleMatch(rhs, ".")) + return ChildrenToVisit::op1_and_op2; + if (!rhs->isBinaryOp() || rhs->astOperand1()->variable() != lhsvar) + return ChildrenToVisit::none; + if (lhsmember->str() == rhs->astOperand2()->str()) + return ChildrenToVisit::none; + errorToken = rhs->astOperand2(); + return ChildrenToVisit::done; + }); + if (errorToken) + overlappingWriteUnion(tok); + } else if (Token::Match(tok, "%name% (")) { + const Library::NonOverlappingData *nonOverlappingData = mSettings->library.getNonOverlappingData(tok); + if (!nonOverlappingData) + continue; + const std::vector args = getArguments(tok); + if (nonOverlappingData->ptr1Arg <= 0 || nonOverlappingData->ptr1Arg > args.size()) + continue; + if (nonOverlappingData->ptr2Arg <= 0 || nonOverlappingData->ptr2Arg > args.size()) + continue; + if (nonOverlappingData->sizeArg <= 0 || nonOverlappingData->sizeArg > args.size()) + continue; + if (!args[nonOverlappingData->sizeArg-1]->hasKnownIntValue()) + continue; + const Token *buf1, *buf2; + MathLib::bigint offset1, offset2; + if (!getBufAndOffset(args[nonOverlappingData->ptr1Arg-1], &buf1, &offset1)) + continue; + if (!getBufAndOffset(args[nonOverlappingData->ptr2Arg-1], &buf2, &offset2)) + continue; + + ErrorPath errorPath; + const bool macro = true; + const bool pure = true; + const bool follow = true; + if (!isSameExpression(mTokenizer->isCPP(), macro, buf1, buf2, mSettings->library, pure, follow, &errorPath)) + continue; + overlappingWriteFunction(tok); + } + } + } +} + +void CheckOther::overlappingWriteUnion(const Token *tok) +{ + reportError(tok, Severity::error, "overlappingWriteUnion", "Overlapping read/write of union is undefined behavior"); +} + +void CheckOther::overlappingWriteFunction(const Token *tok) +{ + const std::string funcname = tok ? tok->str() : ""; + reportError(tok, Severity::error, "overlappingWriteFunction", "Overlapping read/write in " + funcname + "() is undefined behavior"); +} diff --git a/lib/checkother.h b/lib/checkother.h index b3c50871f..4c8dc9cd1 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -102,6 +102,7 @@ public: checkOther.checkMisusedScopedObject(); checkOther.checkAccessOfMovedVariable(); checkOther.checkModuloOfOne(); + checkOther.checkOverlappingWrite(); } /** Is expression a comparison that checks if a nonzero (unsigned/pointer) expression is less than zero? */ @@ -226,6 +227,10 @@ public: void checkModuloOfOne(); + void checkOverlappingWrite(); + void overlappingWriteUnion(const Token *tok); + void overlappingWriteFunction(const Token *tok); + private: // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, const bool result); @@ -298,6 +303,8 @@ private: c.checkPipeParameterSizeError(nullptr, "varname", "dimension"); c.raceAfterInterlockedDecrementError(nullptr); c.invalidFreeError(nullptr, "malloc", false); + c.overlappingWriteUnion(nullptr); + c.overlappingWriteFunction(nullptr); //performance c.redundantCopyError(nullptr, "varname"); @@ -376,6 +383,7 @@ private: "- cast the return values of getc(),fgetc() and getchar() to character and compare it to EOF\n" "- race condition with non-interlocked access after InterlockedDecrement() call\n" "- expression 'x = x++;' depends on order of evaluation of side effects\n" + "- overlapping write of union\n" // warning "- either division by zero or useless condition\n" diff --git a/lib/library.cpp b/lib/library.cpp index e4e723aed..c7c4154c8 100644 --- a/lib/library.cpp +++ b/lib/library.cpp @@ -640,7 +640,13 @@ Library::Error Library::loadFunction(const tinyxml2::XMLElement * const node, co func.isconst = true; // a constant function is pure } else if (functionnodename == "leak-ignore") func.leakignore = true; - else if (functionnodename == "use-retval") { + else if (functionnodename == "not-overlapping-data") { + NonOverlappingData nonOverlappingData; + nonOverlappingData.ptr1Arg = functionnode->IntAttribute("ptr1-arg", -1); + nonOverlappingData.ptr2Arg = functionnode->IntAttribute("ptr2-arg", -1); + nonOverlappingData.sizeArg = functionnode->IntAttribute("size-arg", -1); + mNonOverlappingData[name] = nonOverlappingData; + } else if (functionnodename == "use-retval") { func.useretval = Library::UseRetValType::DEFAULT; if (const char *type = functionnode->Attribute("type")) if (std::strcmp(type, "error-code") == 0) @@ -1265,6 +1271,14 @@ bool Library::formatstr_secure(const Token* ftok) const return functions.at(getFunctionName(ftok)).formatstr_secure; } +const Library::NonOverlappingData* Library::getNonOverlappingData(const Token *ftok) const +{ + if (isNotLibraryFunction(ftok)) + return nullptr; + const std::unordered_map::const_iterator it = mNonOverlappingData.find(getFunctionName(ftok)); + return (it != mNonOverlappingData.cend()) ? &it->second : nullptr; +} + Library::UseRetValType Library::getUseRetValType(const Token *ftok) const { if (isNotLibraryFunction(ftok)) diff --git a/lib/library.h b/lib/library.h index b5e63a8c5..f452852f4 100644 --- a/lib/library.h +++ b/lib/library.h @@ -168,6 +168,13 @@ public: bool formatstr_scan(const Token* ftok) const; bool formatstr_secure(const Token* ftok) const; + struct NonOverlappingData { + int ptr1Arg; + int ptr2Arg; + int sizeArg; + }; + const NonOverlappingData* getNonOverlappingData(const Token *ftok) const; + struct WarnInfo { std::string message; Standards standards; @@ -581,6 +588,7 @@ private: std::map mPlatformTypes; // platform independent typedefs std::map mPlatforms; // platform dependent typedefs std::map, TypeCheck> mTypeChecks; + std::unordered_map mNonOverlappingData; const ArgumentChecks * getarg(const Token *ftok, int argnr) const; diff --git a/test/testother.cpp b/test/testother.cpp index 274890f02..57ccb4131 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -252,6 +252,8 @@ private: TEST_CASE(moduloOfOne); TEST_CASE(sameExpressionPointers); + + TEST_CASE(checkOverlappingWrite); } void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, bool verbose=false, Settings* settings = nullptr) { @@ -9312,6 +9314,35 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); } + + void checkOverlappingWrite() { + // union + check("void foo() {\n" + " union { int i; float f; } u;\n" + " u.i = 0;\n" + " u.i = u.f;\n" // <- error + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Overlapping read/write of union is undefined behavior\n", errout.str()); + + // memcpy + check("void foo() {\n" + " char a[10];\n" + " memcpy(&a[5], &a[4], 2u);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Overlapping read/write in memcpy() is undefined behavior\n", errout.str()); + + check("void foo() {\n" + " char a[10];\n" + " memcpy(a+5, a+4, 2u);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Overlapping read/write in memcpy() is undefined behavior\n", errout.str()); + + check("void foo() {\n" + " char a[10];\n" + " memcpy(a, a+1, 2u);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Overlapping read/write in memcpy() is undefined behavior\n", errout.str()); + } }; REGISTER_TEST(TestOther)