Skip to content

Commit

Permalink
Fix 9298 (#2476)
Browse files Browse the repository at this point in the history
* Fix 9298

Tell cppcheck that strcpy returns its first argument, and use that
knowledge in checkTokenInsideExpression.

* Add missing unit tests in cmake
  • Loading branch information
Ken-Patrick authored and danmar committed Jan 9, 2020
1 parent bca5d0d commit 0b7649c
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 4 deletions.
2 changes: 1 addition & 1 deletion cfg/std.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -4641,7 +4641,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun
</function>
<!-- char *strcpy(char *desstr, const char *srcstr); -->
<function name="strcpy,std::strcpy">
<returnValue type="char *"/>
<returnValue type="char *">arg1</returnValue>
<noreturn>false</noreturn>
<leak-ignore/>
<arg nr="1" direction="out">
Expand Down
29 changes: 26 additions & 3 deletions lib/checkleakautovar.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -743,8 +743,31 @@ const Token * CheckLeakAutoVar::checkTokenInsideExpression(const Token * const t
deallocUseError(tok, tok->str());
} else if (Token::simpleMatch(tok->tokAt(-2), "= &")) {
varInfo->erase(tok->varId());
} else if (Token::Match(tok->previous(), "= %var% [;,)]")) {
varInfo->erase(tok->varId());
} else {
// check if tok is assigned into another variable
const Token *rhs = tok;
while (rhs->astParent()) {
if (rhs->astParent()->str() == "=")
break;
rhs = rhs->astParent();
}
if (rhs->varId() == tok->varId()) {
// simple assignment
varInfo->erase(tok->varId());
} else if (rhs->str() == "(" && mSettings->library.returnValue(rhs->astOperand1()) != emptyString) {
// #9298, assignment through return value of a function
const std::string &returnValue = mSettings->library.returnValue(rhs->astOperand1());
if (returnValue.compare(0, 3, "arg") == 0) {
int argn;
const Token *func = getTokenArgumentFunction(tok, argn);
if (func) {
const std::string arg = "arg" + std::to_string(argn + 1);
if (returnValue == arg) {
varInfo->erase(tok->varId());
}
}
}
}
}
} else if (Token::Match(tok->previous(), "& %name% = %var% ;")) {
varInfo->referenced.insert(tok->tokAt(2)->varId());
Expand All @@ -759,7 +782,7 @@ const Token * CheckLeakAutoVar::checkTokenInsideExpression(const Token * const t
if (alloc.type == 0)
alloc.status = VarInfo::NOALLOC;
functionCall(tok, openingPar, varInfo, alloc, nullptr);
return openingPar->link();
return openingPar;
}

return nullptr;
Expand Down
6 changes: 6 additions & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ if (BUILD_TESTS)
add_fixture(${CMAKE_MATCH_1})
endif()
endforeach()
add_fixture("TestLeakAutoVarStrcpy")
add_fixture("TestLeakAutoVarWindows")
add_fixture("TestMemleakInFunction")
add_fixture("TestMemleakInClass")
add_fixture("TestMemleakStructMember")
add_fixture("TestMemleakNoVar")

function(add_cfg CFG_TEST)
set(options INCONCLUSIVE)
Expand Down
41 changes: 41 additions & 0 deletions test/testleakautovar.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2020,7 +2020,48 @@ class TestLeakAutoVar : public TestFixture {
REGISTER_TEST(TestLeakAutoVar)


class TestLeakAutoVarStrcpy: public TestFixture {
public:
TestLeakAutoVarStrcpy() : TestFixture("TestLeakAutoVarStrcpy") {
}

private:
Settings settings;

void check(const char code[]) {
// Clear the error buffer..
errout.str("");

// Tokenize..
Tokenizer tokenizer(&settings, this);
std::istringstream istr(code);
tokenizer.tokenize(istr, "test.cpp");

// Check for leaks..
CheckLeakAutoVar checkLeak;
settings.checkLibrary = true;
settings.addEnabled("information");
checkLeak.runChecks(&tokenizer, &settings, this);
}

void run() OVERRIDE {
LOAD_LIB_2(settings.library, "std.cfg");

TEST_CASE(returnedValue); // #9298
}

void returnedValue() { // #9298
check("char *m;\n"
"void strcpy_returnedvalue(const char* str)\n"
"{\n"
" char* ptr = new char[strlen(str)+1];\n"
" m = strcpy(ptr, str);\n"
"}");
ASSERT_EQUALS("", errout.str());
}
};

REGISTER_TEST(TestLeakAutoVarStrcpy)


class TestLeakAutoVarWindows : public TestFixture {
Expand Down

3 comments on commit 0b7649c

@amai2012
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to trigger the regression reported at https://trac.cppcheck.net/ticket/9575

@orbitcowboy
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should revert this change, since it introduced FPs.

@Ken-Patrick
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed with #2540

Please sign in to comment.