Commit 1324e2d7 by Victor Costan

Remove multiple inheritance from "unintesting call" mock classes.

Internal CL 156157936, which was published in commit fe402c27, introduced undefined behavior by casting a base class (internal::{Naggy,Nice,Strict}Base<MockClass>, using the curiously recurring template pattern) pointer to a derived class ({Naggy,Nice,Strict}Mock<MockClass>), in the base class' constructor. At that point, the object isn't guaranteed to have taken on the shape of the derived class, and casting is undefined behavior. The undefined behavior was caught by Chrome's CFI build bot [1], and prevents rolling googletest past that commit / CL. This commit simplifies the {Naggy,Nice,Strict}Mock class hierarchy in a way that removes the undefined behavior. [1] https://www.chromium.org/developers/testing/control-flow-integrity
parent fdb57f85
...@@ -83,79 +83,61 @@ $var method=[[$if kind==0 [[AllowUninterestingCalls]] ...@@ -83,79 +83,61 @@ $var method=[[$if kind==0 [[AllowUninterestingCalls]]
$elif kind==1 [[WarnUninterestingCalls]] $elif kind==1 [[WarnUninterestingCalls]]
$else [[FailUninterestingCalls]]]] $else [[FailUninterestingCalls]]]]
namespace internal {
// $clazz[[]]Base serves as a mix-in to establish the "uninteresting call"
// behavior for $clazz on construction. It accomplishes this via CRTP to get
// access to the derived MockClass.
template <class MockClass> template <class MockClass>
class $clazz[[]]Base { class $clazz : public MockClass {
protected:
$clazz[[]]Base();
~$clazz[[]]Base();
};
} // namespace internal
template <class MockClass>
class $clazz : public MockClass, public internal::$clazz[[]]Base<MockClass> {
public: public:
$clazz() : MockClass() {} $clazz() : MockClass() {
::testing::Mock::$method(
internal::ImplicitCast_<MockClass*>(this));
}
#if GTEST_LANG_CXX11 #if GTEST_LANG_CXX11
// Ideally, we would inherit base class's constructors through a using
// declaration, which would preserve their visibility. However, many existing
// tests rely on the fact that current implementation reexports protected
// constructors as public. These tests would need to be cleaned up first.
// Single argument constructor is special-cased so that it can be // Single argument constructor is special-cased so that it can be
// made explicit. // made explicit.
template <typename A> template <typename A>
explicit $clazz(A&& arg) : MockClass(std::forward<A>(arg)) {} explicit $clazz(A&& arg) : MockClass(std::forward<A>(arg)) {
::testing::Mock::$method(
internal::ImplicitCast_<MockClass*>(this));
}
template <typename A1, typename A2, typename... An> template <typename A1, typename A2, typename... An>
$clazz(A1&& arg1, A2&& arg2, An&&... args) $clazz(A1&& arg1, A2&& arg2, An&&... args)
: MockClass(std::forward<A1>(arg1), std::forward<A2>(arg2), : MockClass(std::forward<A1>(arg1), std::forward<A2>(arg2),
std::forward<An>(args)...) {} std::forward<An>(args)...) {
::testing::Mock::$method(
internal::ImplicitCast_<MockClass*>(this));
}
#else #else
// C++98 doesn't have variadic templates, so we have to define one // C++98 doesn't have variadic templates, so we have to define one
// for each arity. // for each arity.
template <typename A1> template <typename A1>
explicit $clazz(const A1& a1) : MockClass(a1) {} explicit $clazz(const A1& a1) : MockClass(a1) {
::testing::Mock::$method(
internal::ImplicitCast_<MockClass*>(this));
}
$range i 2..n $range i 2..n
$for i [[ $for i [[
$range j 1..i $range j 1..i
template <$for j, [[typename A$j]]> template <$for j, [[typename A$j]]>
$clazz($for j, [[const A$j& a$j]]) : MockClass($for j, [[a$j]]) {} $clazz($for j, [[const A$j& a$j]]) : MockClass($for j, [[a$j]]) {
::testing::Mock::$method(
internal::ImplicitCast_<MockClass*>(this));
}
]] ]]
#endif // GTEST_LANG_CXX11 #endif // GTEST_LANG_CXX11
~$clazz() {
::testing::Mock::UnregisterCallReaction(
internal::ImplicitCast_<MockClass*>(this));
}
private: private:
GTEST_DISALLOW_COPY_AND_ASSIGN_($clazz); GTEST_DISALLOW_COPY_AND_ASSIGN_($clazz);
}; };
namespace internal {
template <typename MockClass>
$clazz[[]]Base<MockClass>::$clazz[[]]Base() {
::testing::Mock::$method(
internal::ImplicitCast_<MockClass*>(
static_cast<$clazz<MockClass> *>(this)));
}
template <typename MockClass>
$clazz[[]]Base<MockClass>::~$clazz[[]]Base() {
::testing::Mock::UnregisterCallReaction(
internal::ImplicitCast_<MockClass*>(
static_cast<$clazz<MockClass>*>(this)));
}
} // namespace internal
]] ]]
// The following specializations catch some (relatively more common) // The following specializations catch some (relatively more common)
......
...@@ -103,11 +103,6 @@ class ExpectationTester; ...@@ -103,11 +103,6 @@ class ExpectationTester;
// Base class for function mockers. // Base class for function mockers.
template <typename F> class FunctionMockerBase; template <typename F> class FunctionMockerBase;
// Uninteresting call behavior mixins.
template <typename M> class NiceMockBase;
template <typename M> class NaggyMockBase;
template <typename M> class StrictMockBase;
// Protects the mock object registry (in class Mock), all function // Protects the mock object registry (in class Mock), all function
// mockers, and all expectations. // mockers, and all expectations.
// //
...@@ -408,13 +403,13 @@ class GTEST_API_ Mock { ...@@ -408,13 +403,13 @@ class GTEST_API_ Mock {
friend class internal::FunctionMockerBase; friend class internal::FunctionMockerBase;
template <typename M> template <typename M>
friend class internal::NiceMockBase; friend class NiceMock;
template <typename M> template <typename M>
friend class internal::NaggyMockBase; friend class NaggyMock;
template <typename M> template <typename M>
friend class internal::StrictMockBase; friend class StrictMock;
// Tells Google Mock to allow uninteresting calls on the given mock // Tells Google Mock to allow uninteresting calls on the given mock
// object. // object.
......
...@@ -259,6 +259,13 @@ TEST(NiceMockTest, NonDefaultConstructor10) { ...@@ -259,6 +259,13 @@ TEST(NiceMockTest, NonDefaultConstructor10) {
nice_bar.That(5, true); nice_bar.That(5, true);
} }
TEST(NiceMockTest, AllowLeak) {
NiceMock<MockFoo>* leaked = new NiceMock<MockFoo>;
Mock::AllowLeak(leaked);
EXPECT_CALL(*leaked, DoThis());
leaked->DoThis();
}
#if !GTEST_OS_SYMBIAN && !GTEST_OS_WINDOWS_MOBILE #if !GTEST_OS_SYMBIAN && !GTEST_OS_WINDOWS_MOBILE
// Tests that NiceMock<Mock> compiles where Mock is a user-defined // Tests that NiceMock<Mock> compiles where Mock is a user-defined
// class (as opposed to ::testing::Mock). We had to work around an // class (as opposed to ::testing::Mock). We had to work around an
...@@ -352,6 +359,13 @@ TEST(NaggyMockTest, NonDefaultConstructor10) { ...@@ -352,6 +359,13 @@ TEST(NaggyMockTest, NonDefaultConstructor10) {
naggy_bar.That(5, true); naggy_bar.That(5, true);
} }
TEST(NaggyMockTest, AllowLeak) {
NaggyMock<MockFoo>* leaked = new NaggyMock<MockFoo>;
Mock::AllowLeak(leaked);
EXPECT_CALL(*leaked, DoThis());
leaked->DoThis();
}
#if !GTEST_OS_SYMBIAN && !GTEST_OS_WINDOWS_MOBILE #if !GTEST_OS_SYMBIAN && !GTEST_OS_WINDOWS_MOBILE
// Tests that NaggyMock<Mock> compiles where Mock is a user-defined // Tests that NaggyMock<Mock> compiles where Mock is a user-defined
// class (as opposed to ::testing::Mock). We had to work around an // class (as opposed to ::testing::Mock). We had to work around an
...@@ -426,6 +440,13 @@ TEST(StrictMockTest, NonDefaultConstructor10) { ...@@ -426,6 +440,13 @@ TEST(StrictMockTest, NonDefaultConstructor10) {
"Uninteresting mock function call"); "Uninteresting mock function call");
} }
TEST(StrictMockTest, AllowLeak) {
StrictMock<MockFoo>* leaked = new StrictMock<MockFoo>;
Mock::AllowLeak(leaked);
EXPECT_CALL(*leaked, DoThis());
leaked->DoThis();
}
#if !GTEST_OS_SYMBIAN && !GTEST_OS_WINDOWS_MOBILE #if !GTEST_OS_SYMBIAN && !GTEST_OS_WINDOWS_MOBILE
// Tests that StrictMock<Mock> compiles where Mock is a user-defined // Tests that StrictMock<Mock> compiles where Mock is a user-defined
// class (as opposed to ::testing::Mock). We had to work around an // class (as opposed to ::testing::Mock). We had to work around an
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment