Index: include/clang/Basic/Attr.td =================================================================== --- include/clang/Basic/Attr.td +++ include/clang/Basic/Attr.td @@ -1132,6 +1132,15 @@ let Documentation = [Undocumented]; } +// An attribute indicating that a function/method return value is not safe to be +// treated as bool. +def WarnImpcastToBool : InheritableAttr { + let Spellings = [GCC<"warn_impcast_to_bool">]; + let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag, + "ExpectedFunctionOrMethod">; + let Documentation = [WarnImpcastToBoolDocs]; +} + def AssumeAligned : InheritableAttr { let Spellings = [GCC<"assume_aligned">]; let Subjects = SubjectList<[ObjCMethod, Function]>; Index: include/clang/Basic/AttrDocs.td =================================================================== --- include/clang/Basic/AttrDocs.td +++ include/clang/Basic/AttrDocs.td @@ -2052,6 +2052,23 @@ The ``_Null_unspecified`` nullability qualifier indicates that neither the ``_Nonnull`` nor ``_Nullable`` qualifiers make sense for a particular pointer type. It is used primarily to indicate that the role of null with specific pointers in a nullability-annotated header is unclear, e.g., due to overly-complex implementations or historical factors with a long-lived API. }]; } +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 he knows what he is 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. + } + } + }]; +} def NonNullDocs : Documentation { let Category = NullabilityDocs; Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -54,6 +54,7 @@ FloatZeroConversion]>; def DoublePromotion : DiagGroup<"double-promotion">; +def UnsafeBoolConversion : DiagGroup<"unsafe-bool-conversion">; def EnumTooLarge : DiagGroup<"enum-too-large">; def UnsupportedNan : DiagGroup<"unsupported-nan">; def UnsupportedCB : DiagGroup<"unsupported-cb">; Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -2255,6 +2255,10 @@ "%0 attribute can only be applied once per parameter">; def err_attribute_uuid_malformed_guid : Error< "uuid attribute contains a malformed GUID">; +def warn_attribute_return_int_only : Warning< + "%0 attribute only applies to return values that are integers">, + InGroup; + def warn_attribute_pointers_only : Warning< "%0 attribute only applies to%select{| constant}1 pointer arguments">, InGroup; @@ -2874,6 +2878,10 @@ def warn_impcast_string_literal_to_bool : Warning< "implicit conversion turns string literal into bool: %0 to %1">, InGroup, DefaultIgnore; +def warn_impcast_non_bool_to_bool : Warning< + "implicit conversion turns non-bool into bool: %0 to %1">, + InGroup, DefaultIgnore; + def warn_impcast_different_enum_types : Warning< "implicit conversion from enumeration type %0 to different enumeration type " "%1">, InGroup; Index: include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- include/clang/StaticAnalyzer/Checkers/Checkers.td +++ include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -172,6 +172,9 @@ HelpText<"Check for cases where the dynamic and the static type of an object are unrelated.">, DescFile<"DynamicTypeChecker.cpp">; +def ReturnNonBoolChecker : Checker<"ReturnNonBool">, + HelpText<"Check for dangerous conversion of integral return values to bool.">, + DescFile<"ReturnNonBoolChecker.cpp">; } // end "alpha.core" let ParentPackage = Nullability in { Index: lib/Sema/SemaChecking.cpp =================================================================== --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -8252,6 +8252,24 @@ // Diagnose implicit casts to bool. if (Target->isSpecificBuiltinType(BuiltinType::Bool)) { + /// 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 (isa(E)) { + FunctionDecl* fn = cast(E)->getDirectCallee(); + if (!fn) + return; + 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->getDeclName(); + return; + } + } if (isa(E)) // Warn on string literal to bool. Checks for string literals in logical // and expressions, for instance, assert(0 && "error here"), are Index: lib/Sema/SemaDeclAttr.cpp =================================================================== --- lib/Sema/SemaDeclAttr.cpp +++ lib/Sema/SemaDeclAttr.cpp @@ -1307,6 +1307,21 @@ Attr.getAttributeSpellingListIndex())); } +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) + <addAttr(::new (S.Context) + WarnImpcastToBoolAttr(Attr.getRange(), S.Context, + Attr.getAttributeSpellingListIndex())); +} + static void handleAssumeAlignedAttr(Sema &S, Decl *D, const AttributeList &Attr) { Expr *E = Attr.getArgAsExpr(0), @@ -5537,6 +5552,9 @@ case AttributeList::AT_ReturnsNonNull: handleReturnsNonNullAttr(S, D, Attr); break; + case AttributeList::AT_WarnImpcastToBool: + handleWarnImpcastToBoolAttr(S, D, Attr); + break; case AttributeList::AT_AssumeAligned: handleAssumeAlignedAttr(S, D, Attr); break; Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -65,6 +65,7 @@ PointerSubChecker.cpp PthreadLockChecker.cpp RetainCountChecker.cpp + ReturnNonBoolChecker.cpp ReturnPointerRangeChecker.cpp ReturnUndefChecker.cpp SimpleStreamChecker.cpp Index: lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp =================================================================== --- /dev/null +++ lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp @@ -0,0 +1,145 @@ +//=== 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/ReturnNonBoolTest.c =================================================================== --- /dev/null +++ test/ReturnNonBoolTest.c @@ -0,0 +1,76 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.core.ReturnNonBool -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; +} + +int restricted_wrap(int x) { + int r = NonBool(x); + if (r == -1) { + return 0; + } else { + return r; + } +} + +void test_restricted_wrap() { + // This is a false positive: as the return value has been + // additionally restricted, we cannot say that it is dangerous to cast it to + // bool as we could have handled all the dangerous situation inside restricted + // wrapper. This happens because SVals don't contain constraints and there is + // no good way to get those from the program state. + if (restricted_wrap(2)) // expected-warning{{implicit cast to bool is dangerous for this value}} + return; +} Index: test/ReturnNonBoolTest.cpp =================================================================== --- /dev/null +++ test/ReturnNonBoolTest.cpp @@ -0,0 +1,90 @@ +// RUN: %clang_cc1 -std=c++11 -analyze -analyzer-checker=alpha.core.ReturnNonBool -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 return values that are integers}} + +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) { + //working +} Index: test/ReturnNonBoolTestCompileTime.cpp =================================================================== --- /dev/null +++ test/ReturnNonBoolTestCompileTime.cpp @@ -0,0 +1,39 @@ +// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wunsafe-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 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 InvalidAttributeUsage() RETURNS_NON_BOOL; // expected-warning{{'warn_impcast_to_bool' attribute only applies to return values that are integers}} \ No newline at end of file