google / googletest

GoogleTest - Google Testing and Mocking Framework
https://google.github.io/googletest/
BSD 3-Clause "New" or "Revised" License
33.71k stars 9.98k forks source link

[Bug]: `ReturnRef` of type `shared_ptr<Mock<...>> const&` segfaults #4499

Closed neugepower closed 3 months ago

neugepower commented 3 months ago

Describe the issue

We came across a problem with one of our tests. We have a given interface that has a getter that returns a const& to a shared pointer:

class IDatabase {
public:
    virtual std::shared_ptr<IConnection> const& connection() const = 0;
};

We then defined a mock for this interface type:

class DatabaseMock : public IDatabase {
public:
    MOCK_CONST_METHOD0(connection, std::shared_ptr<IConnection> const&());
};

Lastly, we have defined an expectation on the mocked interface to return another mock:

EXPECT_CALL(*mockDatabase, connection()).WillRepeatedly(ReturnRef(mockConnection));

Given this setup, as soon as we access mockDatabase->connection() the code segfaults. The code does not segfault if instead of returning a mock connection we return a non-mock connection object. Also returning the shared pointer as value instead of const& seems to mitigate the problem (unfortunately we can not change our API easily as it is widely used).

We were wondering if this is a limitation of googletest or a bug. Please see the godbolt link below for the complete and runable example.

Steps to reproduce the problem

https://godbolt.org/z/aKfTecr5v

See last test and commented lines of code. After uncommenting the code the test will segfault.

What version of GoogleTest are you using?

1.10.0 and 1.12.1

What operating system and version are you using?

godbolt & Linux RHEL8

What compiler and version are you using?

gcc 10.5 godbold & gcc version 11.2.1 20220127 (Red Hat 11.2.1-9) (GCC)

What build system are you using?

godbolt & cmake version 3.28.3

Additional context

No response

derekmauro commented 3 months ago

The problem here is that mockConnection is not related to std::shared_ptr<IConnection> const& by inheritance (because of the std::shared_ptr). EXPECT_CALL(*mockDatabase, connection()).WillRepeatedly(ReturnRef(mockConnection)); constructs a temporary std::shared_ptr<IConnection> which does not have the lifetime you expect.

Both Clang and MSVC warn here if you have the proper warnings enabled. For example:

googletest/googlemock/include/gmock/gmock-actions.h:1174:60: error: returning reference to local temporary object [-Werror,-Wreturn-stack-address]
 1174 |     Result Perform(const ArgumentTuple&) override { return ref_; }

Here is one way to write the test in your godbolt link:

TEST(Segv, ReturnRefMockFails)
{
    std::shared_ptr<StrictMock<DatabaseMock>> mockDatabase = std::make_shared<StrictMock<DatabaseMock>>();
    std::shared_ptr<StrictMock<ConnectionMock>> mockConnection = std::make_shared<StrictMock<ConnectionMock>>();
    auto iconn = std::static_pointer_cast<IConnection>(mockConnection);

    EXPECT_CALL(*mockDatabase, check()).WillRepeatedly(Return(true));
    EXPECT_CALL(*mockDatabase, connection()).WillRepeatedly(ReturnRef(iconn));
    EXPECT_CALL(*mockConnection, ok()).WillRepeatedly(Return(true));

    ASSERT_TRUE(mockDatabase->check());
    ASSERT_TRUE(mockDatabase->connection()->ok());
    ASSERT_EQ(mockDatabase->connection(), mockConnection);
}
neugepower commented 3 months ago

Thanks @derekmauro! That is helpful and I will give it a try.