urusant updated this revision to Diff 71921. urusant added a comment. In https://reviews.llvm.org/D24507#546380, @aaron.ballman wrote:
> We try to keep our tests segregated by functionality. e.g., tests relating to > the way the attribute is handled (what it appertains to, args, etc) should > live in Sema, tests relating to the static analyzer behavior should live in > test/Analysis, etc. > > Tests that are still missing are: applying to a non-function type, applying > to a member function, applying to an Obj-C method. For member functions, what > should happen if the function is virtual? What if the overriders do not > specify the attribute? What if an override specifies the attribute but the > base does not? I have added the test cases about member functions. As for ObjC methods, I didn't pay much attention to them while developing the check as ObjC wasn't the primary target. I tried to make a test case for it, and it turned out that it is OK to put an attribute on ObjC method, but you wouldn't get neither compiler warning nor StaticAnalyzer report. That is why I removed ObjC methods from the attribute subjects and replaced the ObjC test case with another one that shows that you cannot apply the attribute to ObjC methods (not sure if it is still necessary, because it seems not very different from applying the attribute to a non-function variable - in both cases we get the same warning). Do you think it's worth digging into how to make it work with ObjC? In this case I might need some help because I don't really speak Objective C. > > > Have you considered making this a type attribute on the return type of > > > the function rather than a declaration attribute on the function > > > declaration? > > > > > > > > > No, I hadn't. On a quick look though, I couldn't find a way to simplify my > > solution using this idea, because as far as I understand, the type > > attribute isn't inherited, so, for example, if I have something like `int r > > = X509_verify_cert(...)` and the function `X509_verify_cert` has a return > > type with attribute, `r` won't have the attribute. If that is correct, we > > still need to backtrace the value to the function declaration. Is there > > something I am missing? > > > I was thinking it would be diagnosed if you attempted to assign from your > attributed type to a type that is not compatible. However, that may still be > problematic because it raises other questions (can you SFINAE on it? > Overload? etc). This might also make the check itself easier (as we don't need path-sensitive analysis), however, it would make the use more complicated as all the users of the dangerous function would have to change their code (even if they are using it correctly). For example, if we refer to the original motivation, annotating dangerous OpenSSL functions would allow us to protect dozens of codebases using them without changing every one of them. 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/Analysis/ReturnNonBoolTest.c test/Analysis/ReturnNonBoolTest.cpp test/SemaCXX/ReturnNonBoolTestCompileTime.cpp test/SemaObjC/ReturnNonBoolTestCompileTime.m
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 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/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/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: 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<[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