From 0950a97df2c143c10d8366e0823f2cfd99da84d2 Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Sun, 6 Oct 2019 00:57:31 -0700 Subject: [PATCH] Fix false negatives in checkBitwiseOnBoolean (#2220) * Fix false negatives in checkBitwiseOnBoolean Use AST-based tests in favor of token-based tests for greater coverage. * Travis: add suppressions for bitwiseOnBool --- .travis_suppressions | 1 + lib/checkbool.cpp | 19 ++++++------------- lib/checkbool.h | 4 ++-- test/testbool.cpp | 25 +++++++++++++++---------- 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/.travis_suppressions b/.travis_suppressions index 65d307bd3..23c9413da 100644 --- a/.travis_suppressions +++ b/.travis_suppressions @@ -6,6 +6,7 @@ noValidConfiguration shadowFunction functionConst functionStatic +bitwiseOnBoolean # temporary suppressions - fix the warnings! duplicateBranch:lib/checkunusedvar.cpp diff --git a/lib/checkbool.cpp b/lib/checkbool.cpp index dc38193ec..c87fef98e 100644 --- a/lib/checkbool.cpp +++ b/lib/checkbool.cpp @@ -93,27 +93,20 @@ void CheckBool::checkBitwiseOnBoolean() const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Scope * scope : symbolDatabase->functionScopes) { for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { - if (Token::Match(tok, "(|.|return|&&|%oror%|throw|, %var% [&|]")) { - const Variable *var = tok->next()->variable(); - if (isBool(var)) { - bitwiseOnBooleanError(tok->next(), var->name(), tok->strAt(2) == "&" ? "&&" : "||"); - tok = tok->tokAt(2); - } - } else if (Token::Match(tok, "[&|] %var% )|.|return|&&|%oror%|throw|,") && (!tok->previous() || !tok->previous()->isExtendedOp() || tok->strAt(-1) == ")" || tok->strAt(-1) == "]")) { - const Variable *var = tok->next()->variable(); - if (isBool(var)) { - bitwiseOnBooleanError(tok->next(), var->name(), tok->str() == "&" ? "&&" : "||"); - tok = tok->tokAt(2); + if (tok->isBinaryOp() && (tok->str() == "&" || tok->str() == "|")) { + if (astIsBool(tok->astOperand1()) || astIsBool(tok->astOperand2())) { + const std::string expression = astIsBool(tok->astOperand1()) ? tok->astOperand1()->expressionString() : tok->astOperand2()->expressionString(); + bitwiseOnBooleanError(tok, expression, tok->str() == "&" ? "&&" : "||"); } } } } } -void CheckBool::bitwiseOnBooleanError(const Token *tok, const std::string &varname, const std::string &op) +void CheckBool::bitwiseOnBooleanError(const Token *tok, const std::string &expression, const std::string &op) { reportError(tok, Severity::style, "bitwiseOnBoolean", - "Boolean variable '" + varname + "' is used in bitwise operation. Did you mean '" + op + "'?", + "Boolean expression '" + expression + "' is used in bitwise operation. Did you mean '" + op + "'?", CWE398, true); } diff --git a/lib/checkbool.h b/lib/checkbool.h index 177e93125..335fe027e 100644 --- a/lib/checkbool.h +++ b/lib/checkbool.h @@ -106,7 +106,7 @@ private: void comparisonOfBoolWithInvalidComparator(const Token *tok, const std::string &expression); void assignBoolToPointerError(const Token *tok); void assignBoolToFloatError(const Token *tok); - void bitwiseOnBooleanError(const Token *tok, const std::string &varname, const std::string &op); + void bitwiseOnBooleanError(const Token *tok, const std::string &expression, const std::string &op); void comparisonOfBoolExpressionWithIntError(const Token *tok, bool n0o1); void pointerArithBoolError(const Token *tok); void returnValueBoolError(const Token *tok); @@ -120,7 +120,7 @@ private: c.comparisonOfTwoFuncsReturningBoolError(nullptr, "func_name1", "func_name2"); c.comparisonOfBoolWithBoolError(nullptr, "var_name"); c.incrementBooleanError(nullptr); - c.bitwiseOnBooleanError(nullptr, "varname", "&&"); + c.bitwiseOnBooleanError(nullptr, "expression", "&&"); c.comparisonOfBoolExpressionWithIntError(nullptr, true); c.pointerArithBoolError(nullptr); c.comparisonOfBoolWithInvalidComparator(nullptr, "expression"); diff --git a/test/testbool.cpp b/test/testbool.cpp index 385a984c5..5b2c0e118 100644 --- a/test/testbool.cpp +++ b/test/testbool.cpp @@ -785,56 +785,61 @@ private: check("void f(_Bool a, _Bool b) {\n" " if(a & b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean variable 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean expression 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); check("void f(_Bool a, _Bool b) {\n" " if(a | b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean variable 'a' is used in bitwise operation. Did you mean '||'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean expression 'a' is used in bitwise operation. Did you mean '||'?\n", errout.str()); check("void f(bool a, bool b) {\n" " if(a & !b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean variable 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean expression 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); check("void f(bool a, bool b) {\n" " if(a | !b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean variable 'a' is used in bitwise operation. Did you mean '||'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean expression 'a' is used in bitwise operation. Did you mean '||'?\n", errout.str()); check("bool a, b;\n" "void f() {\n" " if(a & b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (style, inconclusive) Boolean variable 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style, inconclusive) Boolean expression 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); check("bool a, b;\n" "void f() {\n" " if(a & !b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (style, inconclusive) Boolean variable 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style, inconclusive) Boolean expression 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); check("bool a, b;\n" "void f() {\n" " if(a | b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (style, inconclusive) Boolean variable 'a' is used in bitwise operation. Did you mean '||'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style, inconclusive) Boolean expression 'a' is used in bitwise operation. Did you mean '||'?\n", errout.str()); check("bool a, b;\n" "void f() {\n" " if(a | !b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (style, inconclusive) Boolean variable 'a' is used in bitwise operation. Did you mean '||'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style, inconclusive) Boolean expression 'a' is used in bitwise operation. Did you mean '||'?\n", errout.str()); check("void f(bool a, int b) {\n" " if(a & b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean variable 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean expression 'a' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); check("void f(int a, bool b) {\n" " if(a & b) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean variable 'b' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean expression 'b' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); + + check("void f(int a, int b) {\n" + " if((a > 0) & (b < 0)) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Boolean expression 'a>0' is used in bitwise operation. Did you mean '&&'?\n", errout.str()); check("void f(int a, int b) {\n" " if(a & b) {}\n"