Partial fix for #12468 Library functions not handled in assertWithSid…

…eEffect check (#6364)
danmar · Apr 30, 2024 · 1f0ab48 · 1f0ab48
1 parent e2fa3cb
commit 1f0ab48
Show file tree

Hide file tree

Showing 9 changed files with 52 additions and 5 deletions.
diff --git a/Makefile b/Makefile
@@ -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

diff --git a/cfg/gtk.cfg b/cfg/gtk.cfg
@@ -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/>

diff --git a/lib/checkassert.cpp b/lib/checkassert.cpp
@@ -22,6 +22,7 @@
 
 #include "checkassert.h"
 
+#include "astutils.h"
 #include "errortypes.h"
 #include "settings.h"
 #include "symboldatabase.h"
@@ -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;

diff --git a/lib/library.cpp b/lib/library.cpp
@@ -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
 {

diff --git a/lib/library.h b/lib/library.h
@@ -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();
     }
@@ -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,

diff --git a/oss-fuzz/Makefile b/oss-fuzz/Makefile
@@ -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

diff --git a/test/cfg/gtk.c b/test/cfg/gtk.c
@@ -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);
 }

diff --git a/test/cfg/std.cpp b/test/cfg/std.cpp
@@ -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"));
+}
diff --git a/test/testassert.cpp b/test/testassert.cpp
@@ -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() {