[Bug]: MSVC - Assertion macro SUCCEED() leeds to a memory leak.

[fwidmann]

Describe the issue

To secure my C++ applications under MSVC (V17.10.3), I like to use the functions of the extension "CRT debug heap" from Microsoft. I want to ensure that my software does not leave any memory fragments on the heap. My tests therefore checks whether the heap state is identical before and after a test case.

In most cases, this works very well. However, if I use the assertion macro SUCCEED(), a memory leak is reported.

Steps to reproduce the problem

Use the following test case:

TEST(GoogleTest, TestSucceed)
{
    // ---- store heap state ----
    _CrtMemState s1, s2, s3;
    _CrtMemCheckpoint(&s1);

    // ---- TEST ----

    // EXPECT_TRUE(true);   // (1)
    SUCCEED();              // (2)

    // ---- compare heap state ----
    _CrtMemCheckpoint(&s2);
    bool hasMemoryLeaks = (0 != _CrtMemDifference(&s3, &s1, &s2));
    if (hasMemoryLeaks)
    {
        _CrtMemDumpAllObjectsSince(&s1);
    }
    EXPECT_FALSE(hasMemoryLeaks);
}

The test ends successfully as long as I use the assertion EXPECT_TRUE(true) (marker (1)). As soon as I use the macro SUCCEED() (marker (2)), a memory leak is reported at the end.

What version of GoogleTest are you using?

GoogleTest v1.14.0 commit f8d7d77c06936315286eb55f8de22cd23c188571

What operating system and version are you using?

Microsoft Windows 11 Version 23H2 (Build 22631.3447)

What compiler and version are you using?

Microsoft Visual Studio Professional 2022 (64-bit) Current Version 17.10.3

What build system are you using?

msbuild --version MSBuild-Version 17.10.4+10fbfbf2e für .NET Framework 17.10.4.21802

Additional context

No response

[derekmauro]

I haven't tried this, so I could be wrong, but I think this is probably working as intended.

It looks like under the hood, the SUCCEED() macro allocates memory, and frees it in its destructor. The destructor is not run until after the call to _CrtMemDifference, so I'm guessing this is the source of the difference.

Now we could change SUCCEED() to not allocate memory, but that is not a scalable solution. Any RAII object on the stack is going to have the same problem.

If you really want to use _CrtMemDifference (and I think there are much better memory checkers available, like LSAN), I think you should create an RAII object that saves the state in its constructor and checks for leaks in its destructor. Put this object on the stack and it will ensure that it runs after all objects that were constructed after it have also had their destructors called.

[derekmauro]

By the way, you can test my hypothesis by simply using an additional set of braces around the body of the test:

TEST(GoogleTest, TestSucceed)
{
    // ---- store heap state ----
    _CrtMemState s1, s2, s3;
    _CrtMemCheckpoint(&s1);

    // ---- TEST ----
    {
        SUCCEED();
    }

    // ---- compare heap state ----
    _CrtMemCheckpoint(&s2);
    bool hasMemoryLeaks = (0 != _CrtMemDifference(&s3, &s1, &s2));
    if (hasMemoryLeaks)
    {
        _CrtMemDumpAllObjectsSince(&s1);
    }
    EXPECT_FALSE(hasMemoryLeaks);
}

[fwidmann]

@derekmauro Sorry for the delay, I was out of the office for a few days.

Unfortunately, your suggestion does not bring any improvement.

I would have been surprised too, because in my real project the memory leak check is handled in a separate TestEventListener. So the binding scope of SUCCEED() is already finished when the memory check is called. I have simplified the call structure somewhat just for this issue description.