Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partial fix for #12468 Library functions not handled in assertWithSideEffect check #6364

Merged
merged 16 commits into from
Apr 30, 2024
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ $(libcppdir)/check.o: lib/check.cpp lib/addoninfo.h lib/check.h lib/color.h lib/
$(libcppdir)/check64bit.o: lib/check64bit.cpp lib/addoninfo.h lib/check.h lib/check64bit.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/check64bit.cpp

$(libcppdir)/checkassert.o: lib/checkassert.cpp lib/addoninfo.h lib/check.h lib/checkassert.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
$(libcppdir)/checkassert.o: lib/checkassert.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checkassert.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkassert.cpp

$(libcppdir)/checkautovariables.o: lib/checkautovariables.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checkautovariables.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h
Expand Down
1 change: 1 addition & 0 deletions cfg/gtk.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -4112,6 +4112,7 @@
<!-- int g_strcmp0 (const char *str1, const char *str2); -->
<function name="g_strcmp0">
<leak-ignore/>
<pure/>
<noreturn>false</noreturn>
<returnValue type="int"/>
<use-retval/>
Expand Down
15 changes: 14 additions & 1 deletion lib/checkassert.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include "checkassert.h"

#include "astutils.h"
#include "errortypes.h"
#include "settings.h"
#include "symboldatabase.h"
Expand Down Expand Up @@ -57,8 +58,20 @@ void CheckAssert::assertWithSideEffects()

checkVariableAssignment(tmp, tok->scope());

if (tmp->tokType() != Token::eFunction)
if (tmp->tokType() != Token::eFunction) {
if (const Library::Function* f = mSettings->library.getFunction(tmp)) {
if (f->isconst || f->ispure)
continue;
if (Library::getContainerYield(tmp->next()) != Library::Container::Yield::NO_YIELD) // bailout, assume read access
continue;
if (std::any_of(f->argumentChecks.begin(), f->argumentChecks.end(), [](const std::pair<int, Library::ArgumentChecks>& ac) {
return ac.second.iteratorInfo.container > 0; // bailout, takes iterators -> assume read access
}))
continue;
sideEffectInAssertError(tmp, mSettings->library.getFunctionName(tmp));
}
continue;
}

const Function* f = tmp->function();
const Scope* scope = f->functionScope;
Expand Down
16 changes: 16 additions & 0 deletions lib/library.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1266,6 +1266,22 @@ bool Library::isContainerYield(const Token * const cond, Library::Container::Yie
return false;
}

Library::Container::Yield Library::getContainerYield(const Token* const cond)
{
if (Token::simpleMatch(cond, "(")) {
const Token* tok = cond->astOperand1();
if (tok && tok->str() == ".") {
if (tok->astOperand1() && tok->astOperand1()->valueType()) {
if (const Library::Container *container = tok->astOperand1()->valueType()->container) {
if (tok->astOperand2())
return container->getYield(tok->astOperand2()->str());
}
}
}
}
return Library::Container::Yield::NO_YIELD;
}

// returns true if ftok is not a library function
bool Library::isNotLibraryFunction(const Token *ftok) const
{
Expand Down
5 changes: 3 additions & 2 deletions lib/library.h
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,9 @@ class CPPCHECKLIB Library {
const Token* getContainerFromYield(const Token* tok, Container::Yield yield) const;
const Token* getContainerFromAction(const Token* tok, Container::Action action) const;

static bool isContainerYield(const Token* const cond, Library::Container::Yield y, const std::string& fallback = emptyString);
static Library::Container::Yield getContainerYield(const Token* const cond);

bool isreflection(const std::string &token) const {
return mReflection.find(token) != mReflection.end();
}
Expand Down Expand Up @@ -496,8 +499,6 @@ class CPPCHECKLIB Library {
*/
std::string getFunctionName(const Token *ftok) const;

static bool isContainerYield(const Token * const cond, Library::Container::Yield y, const std::string& fallback=emptyString);

/** Suppress/check a type */
enum class TypeCheck { def,
check,
Expand Down
2 changes: 1 addition & 1 deletion oss-fuzz/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ $(libcppdir)/check.o: ../lib/check.cpp ../lib/addoninfo.h ../lib/check.h ../lib/
$(libcppdir)/check64bit.o: ../lib/check64bit.cpp ../lib/addoninfo.h ../lib/check.h ../lib/check64bit.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/settings.h ../lib/sourcelocation.h ../lib/standards.h ../lib/suppressions.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/vfvalue.h
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/check64bit.cpp

$(libcppdir)/checkassert.o: ../lib/checkassert.cpp ../lib/addoninfo.h ../lib/check.h ../lib/checkassert.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/settings.h ../lib/sourcelocation.h ../lib/standards.h ../lib/suppressions.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/vfvalue.h
$(libcppdir)/checkassert.o: ../lib/checkassert.cpp ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checkassert.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/suppressions.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/vfvalue.h
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkassert.cpp

$(libcppdir)/checkautovariables.o: ../lib/checkautovariables.cpp ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checkautovariables.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/suppressions.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/valueflow.h ../lib/vfvalue.h
Expand Down
1 change: 1 addition & 0 deletions test/cfg/gtk.c
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ void validCode(int argInt, GHashTableIter * hash_table_iter, GHashTable * hash_t
// NULL is handled graciously
char* str = g_strdup(NULL);
if (g_strcmp0(str, NULL) || g_strcmp0(NULL, str))
// cppcheck-suppress valueFlowBailout // TODO: caused by <pure/>?
printf("%s", str);
g_free(str);
}
Expand Down
6 changes: 6 additions & 0 deletions test/cfg/std.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5016,3 +5016,9 @@ void eraseIteratorOutOfBounds_std_deque(std::deque<int>& x) // #8690
// cppcheck-suppress eraseIteratorOutOfBounds
x.erase(x.end());
}

void assertWithSideEffect_system()
{
// cppcheck-suppress [assertWithSideEffect,checkLibraryNoReturn] // TODO: #8329
assert(std::system("abc"));
}
9 changes: 9 additions & 0 deletions test/testassert.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,15 @@ class TestAssert : public TestFixture {
" assert(empty());\n"
"}");
ASSERT_EQUALS("", errout_str());

check("struct S {\n" // #4811
" void f() const;\n"
" bool g(std::ostream& os = std::cerr) const;\n"
"};\n"
"void S::f() const {\n"
" assert(g());\n"
"}\n");
ASSERT_EQUALS("", errout_str());
}

void memberFunctionCallInAssert() {
Expand Down
Loading