Index: include/clang/Basic/Attr.td =================================================================== --- include/clang/Basic/Attr.td +++ include/clang/Basic/Attr.td @@ -1134,0 +1135,8 @@ +// An attribute indicating that a function/method return value is not safe to be +// treated as bool. +def WarnImpcastToBool : InheritableAttr { + let Spellings = [GNU<"warn_impcast_to_bool">]; + let Subjects = SubjectList<[Function]>; + let Documentation = [WarnImpcastToBoolDocs]; +} + Index: include/clang/Basic/AttrDocs.td =================================================================== --- include/clang/Basic/AttrDocs.td +++ include/clang/Basic/AttrDocs.td @@ -2054,0 +2055,25 @@ +def WarnImpcastToBoolDocs : Documentation { + let Category = DocCatFunction; + let Content = [{ + The ``warn_impcast_to_bool`` attribute is used to indicate that the return + value of a function with integral return type cannot be used as a boolean + value. For example, if a function returns -1 if it couldn't efficiently read + the data, 0 if the data is invalid and 1 for success, it might be dangerous + to implicitly cast the return value to bool, e.g. to indicate success. + Therefore, it is a good idea to trigger a warning about such cases. However, + in case a programmer uses an explicit cast to bool, that probably means that + they know what they are doing, therefore a warning should be triggered only + for implicit casts. + + .. code-block:: c + + int f(int x) __attribute__((warn_impcast_to_bool)); + + void test(int x) { + if (f(x)) { // diagnoses + } + if ((bool)f(x)) { // Does not diagnose, explicit cast. + } + } + }]; +} Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -2257,0 +2258,4 @@ +def warn_attribute_return_int_only : Warning< + "%0 attribute only applies to integer return types">, + InGroup; + @@ -2876,0 +2881,4 @@ +def warn_impcast_non_bool_to_bool : Warning< + "implicit conversion turns non-bool into bool: %0 to %1">, + InGroup; + Index: include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- include/clang/StaticAnalyzer/Checkers/Checkers.td +++ include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -174,0 +175,3 @@ +def ReturnNonBoolChecker : Checker<"ReturnNonBool">, + HelpText<"Check for dangerous conversion of integral return values to bool.">, + DescFile<"ReturnNonBoolChecker.cpp">; Index: lib/Sema/SemaChecking.cpp =================================================================== --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -8254,0 +8255,16 @@ + /// Warn if the expression is the return value of a function call being + /// implicitly cast to bool, while it's specified that it shouldn't be by a + /// 'warn_impcast_to_bool' attribute. + /// + /// Note that this isn't triggered if the function call is part of a more + /// complicated expression, which in turn is cast to bool, + /// e.g. (x ? f : g)(y) + if (const auto *CE = dyn_cast(E)) { + if (const auto Fn = CE->getDirectCallee()) { + if (Fn->hasAttr()) { + DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_non_bool_to_bool); + S.Diag(Fn->getLocation(), diag::note_entity_declared_at) << Fn; + return; + } + } + } Index: lib/Sema/SemaDeclAttr.cpp =================================================================== --- lib/Sema/SemaDeclAttr.cpp +++ lib/Sema/SemaDeclAttr.cpp @@ -1309,0 +1310,14 @@ +static void handleWarnImpcastToBoolAttr(Sema &S, Decl *D, + const AttributeList &Attr) { + QualType ResultType = getFunctionOrMethodResultType(D); + SourceRange SR = getFunctionOrMethodResultSourceRange(D); + if (!ResultType->isIntegralOrEnumerationType()) { + S.Diag(Attr.getLoc(), diag::warn_attribute_return_int_only) + << Attr.getName() << SR; + return; + } + + D->addAttr(::new (S.Context) WarnImpcastToBoolAttr( + Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex())); +} + @@ -5539,0 +5554,3 @@ + case AttributeList::AT_WarnImpcastToBool: + handleWarnImpcastToBoolAttr(S, D, Attr); + break; Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -67,0 +68 @@ + ReturnNonBoolChecker.cpp Index: lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp =================================================================== --- /dev/null +++ lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp @@ -0,0 +1,144 @@ +//=== ReturnNonBoolChecker.cpp - Non-boolean returns checker ----*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This defines ReturnNonBoolChecker that warns about dangerous conversions of +// integral return values to bool. Such impcast is considered dangerous if this +// is specified by a warn_impcast_to_bool attribute. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/AST/ParentMap.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { +class ReturnNonBoolChecker + : public Checker, + check::PreStmt, check::PostCall> { + mutable std::unique_ptr BT_impcast; + void checkExprValue(CheckerContext &C, const Expr *E) const; + +public: + void checkBranchCondition(const Stmt *Condition, CheckerContext &C) const; + void checkPreStmt(const ImplicitCastExpr *ICE, CheckerContext &C) const; + void checkPreStmt(const UnaryOperator *UO, CheckerContext &C) const; + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; +}; +} // end anonymous namespace + +// A set of values specified to be dangerous for conversion to bool. +REGISTER_SET_WITH_PROGRAMSTATE(NonBoolValues, SymbolRef) + +void ReturnNonBoolChecker::checkExprValue(CheckerContext &C, + const Expr *E) const { + ProgramStateRef State = C.getState(); + SymbolRef SR = State->getSVal(E, C.getLocationContext()).getAsSymbol(); + // If the value isn't marked as dangerous to be cast to bool, no warning is + // needed. + if (!State->contains(SR)) + return; + + ExplodedNode *N = C.generateErrorNode(C.getState()); + if (!N) + return; + if (!BT_impcast) + BT_impcast.reset(new BuiltinBug( + this, "implicit cast to bool is dangerous for this value")); + C.emitReport(llvm::make_unique(*BT_impcast, + BT_impcast->getDescription(), N)); +} + +// This catches 'conversion to bool'-like cases in C, where there is no boolean +// type or implicit cast to bool. +void ReturnNonBoolChecker::checkBranchCondition(const Stmt *Condition, + CheckerContext &C) const { + const Expr *E = dyn_cast(Condition); + if (!E) + return; + const Type *TypePtr = E->getType().getCanonicalType().getTypePtr(); + // If the expression is boolean, then either it is fine to use it or it is + // a cast expression. If it is an explicit cast, it is fine because this means + // that author knows what he is doing, otherwise it will be caught by + // checkPreStmt, so we don't need to do anything here. + if (!TypePtr->isSpecificBuiltinType(BuiltinType::Bool)) + checkExprValue(C, E); +} + +// Checks if the parent of the specified implicit cast in the AST is a CastExpr. +static bool hasParentCast(const ImplicitCastExpr *ICE, + const LocationContext *LC) { + const Stmt *Parent = LC->getParentMap().getParent(ICE); + return Parent && dyn_cast(Parent); +} + +// Checks if the given cast expression is an integral to boolean cast. +static bool isIntToBoolCast(const CastExpr *CE) { + const Type *TypePtr = CE->getType().getCanonicalType().getTypePtr(); + const Type *OrigTypePtr = + CE->getSubExpr()->getType().getCanonicalType().getTypePtr(); + return OrigTypePtr->isIntegralOrEnumerationType() && + TypePtr->isSpecificBuiltinType(BuiltinType::Bool); +} + +// This catches implicit casts from integral types to bool in case they exist, +// i.e. for C++. +void ReturnNonBoolChecker::checkPreStmt(const ImplicitCastExpr *ICE, + CheckerContext &C) const { + // Some types of casts (e.g. C-style cast) have implicit cast as a child, + // which we don't really care about because in this case this is actually an + // explicit cast, which means that the programmer is aware that it's + // dangerous, but still wants to do it. + if (hasParentCast(ICE, C.getLocationContext())) + return; + // This checker is only for integral to boolean casts. + if (!isIntToBoolCast(dyn_cast(ICE))) + return; + checkExprValue(C, ICE->getSubExpr()); +} + +// This is to catch logical negotiation operator, which is an int->int operator +// in C, so it is not caught by either of the two previous methods. +void ReturnNonBoolChecker::checkPreStmt(const UnaryOperator *UO, + CheckerContext &C) const { + if (UO->getOpcode() != UO_LNot) + return; + const Expr *E = UO->getSubExpr(); + const Type *TypePtr = E->getType().getCanonicalType().getTypePtr(); + if (!TypePtr->isIntegralOrEnumerationType()) + return; + checkExprValue(C, E); +} + +// Store the symbolic reference for return value of "ReturnsNonBool" function +// in the set. +void ReturnNonBoolChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + const Decl *Function = Call.getDecl(); + if (!Function || !Function->hasAttr()) + return; + SymbolRef ReturnValue = Call.getReturnValue().getAsSymbol(); + // If the return value cannot be taken as symbol, we don't want to add it + // to the set because it can be achieved by many different ways, and we don't + // want them to be treated as equals. + if (!ReturnValue) + return; + ProgramStateRef State = C.getState(); + State = State->add(ReturnValue); + C.addTransition(State); + return; +} + +void ento::registerReturnNonBoolChecker(CheckerManager &mgr) { + mgr.registerChecker(); +} Index: test/Analysis/ReturnNonBoolTest.c =================================================================== --- /dev/null +++ test/Analysis/ReturnNonBoolTest.c @@ -0,0 +1,79 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.core.ReturnNonBool -Wno-bool-conversion -verify %s + +/// C is checked slightly differently than C++, in particular, C doesn't have +/// implicit casts to bool, so we need to test different branching situations, +/// like if, for, while, etc. + +#ifdef __clang__ +#define RETURNS_NON_BOOL __attribute__((warn_impcast_to_bool)) +#else +#define RETURNS_NON_BOOL +#endif + +int NonBool(int x) RETURNS_NON_BOOL; + +void test_if() { + if (NonBool(2)) // expected-warning{{implicit cast to bool is dangerous for this value}} + return; +} + +void test_while() { + while (NonBool(2)) // expected-warning{{implicit cast to bool is dangerous for this value}} + continue; +} + +void test_for() { + for (; NonBool(2);) // expected-warning{{implicit cast to bool is dangerous for this value}} + continue; +} + +void test_and(int x, int y) { + if (NonBool(2) && (x == y)) // expected-warning{{implicit cast to bool is dangerous for this value}} + return; +} + +void test_or() { + if (NonBool(2) || (1 != 1)) // expected-warning{{implicit cast to bool is dangerous for this value}} + return; +} + +void test_not() { + if (!NonBool(2)) // expected-warning{{implicit cast to bool is dangerous for this value}} + return; +} + +int test_ternary() { + return NonBool(2) ? 1 : 0; // expected-warning{{implicit cast to bool is dangerous for this value}} +} + +int wrap(int x) { + int r = NonBool(x); + return r; +} + +void test_wrap() { + if (wrap(2)) // expected-warning{{implicit cast to bool is dangerous for this value}} + return; +} + +// Example inspired by CVE-2008-5077: +// Returns 1 on success, 0 on failure and something negative on catastrophic +// failure +int verify_cert() __attribute__((warn_impcast_to_bool)); + +void correctly_handled() { + int rc = verify_cert(); + + if (rc < 0) + // error handling + + if (rc) { // expected-warning{{implicit cast to bool is dangerous for this value}} + // Here we unfortunately get a warning although the code does correctly + // handle the documented return codes. However, the static analysis checker + // can't read the comment (or the manpage)... + } + // However, the warning can be easily suppressed, for example, like this: + if (rc != 0) { + } + // In C++ you can use explicit cast to bool as well. +} Index: test/Analysis/ReturnNonBoolTest.cpp =================================================================== --- /dev/null +++ test/Analysis/ReturnNonBoolTest.cpp @@ -0,0 +1,88 @@ +// RUN: %clang_cc1 -std=c++11 -analyze -analyzer-checker=alpha.core.ReturnNonBool -Wno-bool-conversion -verify %s +#ifdef __clang__ +#define RETURNS_NON_BOOL __attribute__((warn_impcast_to_bool)) +#else +#define RETURNS_NON_BOOL +#endif + +int NoAttributes() { return 2; } + +int NonBool(int x) RETURNS_NON_BOOL; + +int good(int x); + +int wrap(int x) { + int r = NonBool(x); + return r; +} + +void test1() { + if (NonBool(1)) { // expected-warning{{implicit cast to bool is dangerous for this value}} + return; + } +} + +void test2() { + if (wrap(2)) { // expected-warning{{implicit cast to bool is dangerous for this value}} + return; + } +} + +void test3() { + if ((bool)NonBool(3)) { // no warning, explicit cast + return; + } +} + +void test4(int x) { + if (bool(wrap(2 * x))) { // no warning, explicit cast + return; + } +} + +void test5() { + if (good(5)) { // no warning, return value isn't marked as dangerous + return; + } +} + +void test6() { + if (good(wrap(2))) { // no warning, wrap is treated as int, not as bool + return; + } +} + +double InvalidAttributeUsage() + RETURNS_NON_BOOL; // expected-warning{{'warn_impcast_to_bool' attribute only applies to integer return types}} + +void test_function_pointer(void (*f)()) { + // This is to test the case when Call.getDecl() returns NULL, because f() + // doesn't have a declaration + f(); +} + +bool universal_bool_wrapper(int (*f)(int), int x) { + // When we call universal_bool_wrapper from test_universal_bool_wrapper, the + // analyzer follows the path and detects that in this line we are doing + // something wrong (assuming that f is actually NonBool). So if we didn't call + // universal_bool_wrapper with any dangerous function, there would be no + // warning. + return f(x); // expected-warning {{implicit cast to bool is dangerous for this value}} +} + +int universal_int_wrapper(int (*f)(int), int x) { return f(x); } + +void test_universal_bool_wrapper(int x) { + if (universal_bool_wrapper(NonBool, x)) return; +} + +void test_universal_int_wrapper(int x) { + if (universal_int_wrapper(NonBool, x)) // expected-warning{{implicit cast to bool is dangerous for this value}} + return; +} + +void test_lambdas(int x) { + if ([](int a) __attribute__((warn_impcast_to_bool))-> int{ return a; }(x)) { // expected-warning{{implicit cast to bool is dangerous for this value}} + } +} + Index: test/SemaCXX/ReturnNonBoolTestCompileTime.cpp =================================================================== --- /dev/null +++ test/SemaCXX/ReturnNonBoolTestCompileTime.cpp @@ -0,0 +1,70 @@ +// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wbool-conversion -verify %s +#ifdef __clang__ +#define RETURNS_NON_BOOL __attribute__((warn_impcast_to_bool)) +#else +#define RETURNS_NON_BOOL +#endif + +int NoAttributes() { return 2; } + +int NonBool(int x) RETURNS_NON_BOOL; +int NonBool(int x) { // expected-note{{'NonBool' declared here}} + return x * 2; +} + +int good(int x) { return x * 2; } + +void test1() { + if (NonBool(2)) { // expected-warning{{implicit conversion turns non-bool into bool: 'int' to 'bool'}} + return; + } +} + +void test3() { + if ((bool)NonBool(2)) { // no warning, explicit cast + return; + } +} + +void test5() { + if (good(2)) { // no warning, return value isn't marked as dangerous + return; + } +} + +double InvalidReturnType() RETURNS_NON_BOOL; // expected-warning{{'warn_impcast_to_bool' attribute only applies to integer return types}} + +int AttributeWithArguments() __attribute__((warn_impcast_to_bool(2))); // expected-error {{'warn_impcast_to_bool' attribute takes no arguments}} + +int NonFunction RETURNS_NON_BOOL; // expected-warning{{'warn_impcast_to_bool' attribute only applies to functions}} + +class Base { + int data; + public: + virtual int SafeMember(); + virtual int NonBoolMember() RETURNS_NON_BOOL; +}; + +class DerivedCorrect : public Base { + public: + int SafeMember(); + int NonBoolMember() RETURNS_NON_BOOL; // expected-note{{'NonBoolMember' declared here}} +}; + +class DerivedIncorrect : public Base { + public: + int SafeMember() RETURNS_NON_BOOL; // expected-note {{'SafeMember' declared here}} + int NonBoolMember(); +}; + +void TestMemberFunctions() { + DerivedCorrect DC; + DerivedIncorrect DI; + if (DC.SafeMember()) {} + if (DI.SafeMember()) {} // expected-warning{{implicit conversion turns non-bool into bool: 'int' to 'bool'}} + if (DC.NonBoolMember()) {} // expected-warning{{implicit conversion turns non-bool into bool: 'int' to 'bool'}} + if (DI.NonBoolMember()) {} +} + + + Index: test/SemaObjC/ReturnNonBoolTestCompileTime.m =================================================================== --- /dev/null +++ test/SemaObjC/ReturnNonBoolTestCompileTime.m @@ -0,0 +1,6 @@ +// RUN: %clang_cc1 -fsyntax-only -verify %s + +@interface INTF +- (int) fee __attribute__((warn_impcast_to_bool)); // expected-warning{{'warn_impcast_to_bool' attribute only applies to functions}} ++ (int) c __attribute__((warn_impcast_to_bool)); // expected-warning{{'warn_impcast_to_bool' attribute only applies to functions}} +@end