urusant updated this revision to Diff 71807. urusant added a comment. Made some changes based on the comments. Please refer to the replies below.
Repository: rL LLVM https://reviews.llvm.org/D24507 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/StaticAnalyzer/Checkers/Checkers.td lib/Sema/SemaChecking.cpp lib/Sema/SemaDeclAttr.cpp lib/StaticAnalyzer/Checkers/CMakeLists.txt lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp test/ReturnNonBoolTest.c test/ReturnNonBoolTest.cpp test/ReturnNonBoolTestCompileTime.cpp
Index: test/ReturnNonBoolTestCompileTime.cpp =================================================================== --- /dev/null +++ test/ReturnNonBoolTestCompileTime.cpp @@ -0,0 +1,37 @@ +// 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}} Index: test/ReturnNonBoolTest.cpp =================================================================== --- /dev/null +++ test/ReturnNonBoolTest.cpp @@ -0,0 +1,87 @@ +// 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/ReturnNonBoolTest.c =================================================================== --- /dev/null +++ test/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: lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp =================================================================== --- /dev/null +++ lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp @@ -0,0 +1,134 @@ +//=== 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::BranchCondition, check::PreStmt<ImplicitCastExpr>, + check::PreStmt<UnaryOperator>, check::PostCall> { + mutable std::unique_ptr<BuiltinBug> 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<NonBoolValues>(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<BugReport>(*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<Expr>(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<ImplicitCastExpr>, 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<CastExpr>(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<CastExpr>(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<WarnImpcastToBoolAttr>()) 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<NonBoolValues>(ReturnValue); + C.addTransition(State); + return; +} + +void ento::registerReturnNonBoolChecker(CheckerManager &mgr) { + mgr.registerChecker<ReturnNonBoolChecker>(); +} 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/Sema/SemaDeclAttr.cpp =================================================================== --- lib/Sema/SemaDeclAttr.cpp +++ lib/Sema/SemaDeclAttr.cpp @@ -1307,6 +1307,20 @@ 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) + << Attr.getName() << SR; + return; + } + + D->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 +5551,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/Sema/SemaChecking.cpp =================================================================== --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -8252,6 +8252,22 @@ // 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 (const auto *CE = dyn_cast<CallExpr>(E)) { + if (const auto Fn = CE->getDirectCallee()) { + if (Fn->hasAttr<WarnImpcastToBoolAttr>()) { + DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_non_bool_to_bool); + S.Diag(Fn->getLocation(), diag::note_entity_declared_at) << Fn; + return; + } + } + } if (isa<StringLiteral>(E)) // Warn on string literal to bool. Checks for string literals in logical // and expressions, for instance, assert(0 && "error here"), are 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: 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 integer return types">, + InGroup<IgnoredAttributes>; + def warn_attribute_pointers_only : Warning< "%0 attribute only applies to%select{| constant}1 pointer arguments">, InGroup<IgnoredAttributes>; @@ -2874,6 +2878,10 @@ def warn_impcast_string_literal_to_bool : Warning< "implicit conversion turns string literal into bool: %0 to %1">, InGroup<StringConversion>, DefaultIgnore; +def warn_impcast_non_bool_to_bool : Warning< + "implicit conversion turns non-bool into bool: %0 to %1">, + InGroup<BoolConversion>; + def warn_impcast_different_enum_types : Warning< "implicit conversion from enumeration type %0 to different enumeration type " "%1">, InGroup<EnumConversion>; Index: include/clang/Basic/AttrDocs.td =================================================================== --- include/clang/Basic/AttrDocs.td +++ include/clang/Basic/AttrDocs.td @@ -2052,6 +2052,31 @@ 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 + 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. + } + } + }]; +} def NonNullDocs : Documentation { let Category = NullabilityDocs; Index: include/clang/Basic/Attr.td =================================================================== --- include/clang/Basic/Attr.td +++ include/clang/Basic/Attr.td @@ -1132,6 +1132,14 @@ 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 = [GNU<"warn_impcast_to_bool">]; + let Subjects = SubjectList<[ObjCMethod, Function]>; + let Documentation = [WarnImpcastToBoolDocs]; +} + def AssumeAligned : InheritableAttr { let Spellings = [GCC<"assume_aligned">]; let Subjects = SubjectList<[ObjCMethod, Function]>;
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits