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
