urusant created this revision.
urusant added reviewers: zaks.anna, dcoughlin, jordan_rose, NoQ.
urusant added subscribers: cfe-commits, daviddrysdale.
urusant set the repository for this revision to rL LLVM.
Herald added subscribers: mgorny, beanz.
Hi,
I am interested in feedback on a patch I was working on that adds a new
attribute (warn_impcast_to_bool) to indicate that the return value of a
function shouldn't be used as a boolean, as well as a compile warning and a
StaticAnalyzer checker to warn about misusing functions with this attribute.
I'd also appreciate any suggestions for how to deal with a class of false
positives that the static analysis checker produces.
The change was originally inspired by CVE-2008-5077 [1], which was the result
of an odd design choice in OpenSSL: having an API that returns 1 for success, 0
for failure... and -1 for catastrophic failure. Various API users fell into the
trap of treating the return value as a boolean, so the patch adds an attribute
to allow this to trigger a warning.
As well as generating a compile-time warning, the patch also includes a new
static analyzer checker to catch more indirect uses, where the non-boolean
integer value gets propagated via function wrappers or local variables.
However, it gives a false positive for cases when the code using the return
value actually checks the value in a non-boolean way (because the SVal doesn't
reflect the fact that the value has been further constrained). I couldn't see
an obvious way to get anything relevant from the RangeConstraintManager; any
suggestions?
To test the check (beyond the included unit tests), I annotated dangerous
OpenSSL functions and tried building 8 OpenSSL-using codebases with it. So far,
this didn't give many results for them: the only possible problem was found in
ruby2.1, which was already fixed a few months ago. However, this change is
still potentially useful - even 7 years after the original CVE, there are still
codebases that fall into OpenSSL's API trap.
[1] https://www.openssl.org/news/secadv/20090107.txt
Repository:
rL LLVM
https://reviews.llvm.org/D24507
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticGroups.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,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
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/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: 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::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,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)
+ <<Attr.getName() << SourceRange() << 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 +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/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<CallExpr>(E)) {
+ FunctionDecl* fn = cast<CallExpr>(E)->getDirectCallee();
+ if (!fn)
+ return;
+ 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->getDeclName();
+ 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 return values that are integers">,
+ 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<UnsafeBoolConversion>, DefaultIgnore;
+
def warn_impcast_different_enum_types : Warning<
"implicit conversion from enumeration type %0 to different enumeration type "
"%1">, InGroup<EnumConversion>;
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/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/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]>;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
