C++: #include <cstdlib> shadows macros from "MemoryLeakDetectorMallocMacros.h"

[Knut Aksel Røysland]

Hi again.

I have been struggling with getting the CppUTest memory leak detector
to play nicely with C++ in certain situations. In my case, a function
from a C file was returning a malloc'ed pointer to be free'd from code
compiled under C++. The problem was that the "malloc" call was
correctly tracked by the memory leak detector, but the "free" call in
the C++ code for some reason ended up being a vanilla call to free(),
causing a false leak detection.

I then made the discovery that <cstdlib>, which in my case was
included via <algorithm> that I was using, starts by #undef'ing
"malloc" and "free" etc., effectively disabling the memory leak
detector inside C++ files including <cstdlib>.

To avoid this from happening, I suggest including <cstdlib> before the
macros are defined, from within "MemoryLeakDetectorMallocMacros.h",
e.g. like this:

--- include/CppUTest/MemoryLeakDetectorMallocMacros.h (revision 731)
+++ include/CppUTest/MemoryLeakDetectorMallocMacros.h (working copy)
@@ -32,6 +32,7 @@
#include "StandardCLibrary.h"

#ifdef __cplusplus
+#include <cstdlib>
extern "C"
{
#endif

... or possibly even better, from "StandardCLibrary.h". Maybe also
putting a check on not CPPUTEST_STD_CPP_LIB_DISABLED around the
include of <cstdlib> is a good thing.

I am looking forward to my revitalized friendship with the CppUTest
memory leak detector after this discovery. :-)

--
Thanks,
Knut Aksel Røysland

[Terry Yin]

Hi,

Cool.

BTW. It took my several googles to find out what "vanilla call" means. So it just means "ordinary plain call". Am I right?

br, Terry

--

-terry

-------------------------
Blog: http://terryyin.blogbus.com/
twitter: http://twitter.com/terryyin

[Bas Vodde]

HAH :)

That is a nice find!

I've been trying to write a test in CppUTest to fail because of this, but I'm failing to write this test!

The reason is that when you actually do a -include with the MemoryLeakDetectorNewMacros.h then that will ensure that <cstdlib> is included already. This is done via the <memory> include that is there. When I add a #error on the cstdlib before the #undef of the malloc then it will blow up already *before* the malloc macro has been defined....

Do you have any idea why that isn't the case for you?

Bas

ps. I want to commit the change, but would love to have a failing test before committing it :(

[Knut Aksel Røysland]

Hi, Bas.

I certainly agree with your desire to have this backed by a test.
After all, that is at the heart of why we believe in testing and
CppUTest. :-)

It appears that the inclusion of <memory> is not sufficient to get
<cstdlib> included in the version of libstdc++ that I am running
(v4.5.2 / Ubuntu 11.04), but this might vary, so maybe on your system,
you would never have gotten into the trouble I experienced. Anyway, I
have attached a test that fails on a false memory leak detection on my
system. Please, feel free to clean it up so it blends in with the rest
of the tests coding-style-wise.

--
Thanks,
Knut Aksel

2012/5/26 Bas Vodde <ba...@odd-e.com>:

[Knut Aksel Røysland]

Hi Terry.

Yes, you are right. Pardon my slang. :-)

--
Thanks,
Knut Aksel

2012/5/26 Terry Yin <terry....@gmail.com>:

[Bas Vodde]

Oki, got it ;)

Indeed.... MacOSX was an exception, I got my earlier test to fail now on Ubuntu too :)

And... my fix didn't work. I'll be looking at it later today.

Tnx

Bas

> <failing-test.diff>

[Bas Vodde]

Hiya Knut Aksel,

I checked in some code that ought to fix this. I did ended up putting it in StandardC.h although I wasn't comfortable with that.

Also added a test of myself.

The test is easier if you do the following:

void* memory = cpputest_malloc(1);
free(memory);

and

void* memory = malloc(1);
cpputest_free(memory);

These will fail when you I didn't add the cstdlib to StandardC.h (except on MacOSX :)

Thanks for this fix. It is a painful one to find!

Bas

> <failing-test.diff>

[Knut Aksel Røysland]

Hi again.

Thanks for looking into this. As of trunk r735, the attached
additional diff makes things work for me. The include of
"StandardCLibrary.h" happened too early, before
"CPPUTEST_USE_STD_CPP_LIB" was set.

About your discomfort with putting the missing include into
"StandardCLibrary.h", I think it belongs there by symmetry as long as
that is where <stdlib.h> is included in case of non-C++. Of course,
you could rename that file to e.g. "StandardLibrary.h" to clarify that
it also does stuff related to the standard C++ library.

--
Knut Aksel

2012/5/27 Bas Vodde <ba...@odd-e.com>:

[Terry Yin]

Bas,
I think the test is ok and enough to drive the code. although it would not file on Mac but it did fail on some other platforms. I think that counts.

[Bas Vodde]

Oh!

I actually had fixed that already.... but.... my svn client was waiting for me to type a password :) ... for... 2 hours :)

(I worked off my usual box)

Now I finally comitted the changes at r736. There was a bit more that was needed to get the test to work also for when the files aren't added via -include.

> Thanks for looking into this. As of trunk r735, the attached
> additional diff makes things work for me. The include of
> "StandardCLibrary.h" happened too early, before
> "CPPUTEST_USE_STD_CPP_LIB" was set.
>
> About your discomfort with putting the missing include into
> "StandardCLibrary.h", I think it belongs there by symmetry as long as
> that is where <stdlib.h> is included in case of non-C++. Of course,
> you could rename that file to e.g. "StandardLibrary.h" to clarify that
> it also does stuff related to the standard C++ library.

Yah, I agree. One problem I had was that the only reason for adding it there is the #undef, so therefore it somehow feels better together with the other MemoryLeakDetector stuff. Anyways, for now I've left it there :)

Bas

> <fix.diff>

[Knut Aksel Røysland]

Hi Terry.

I agree. This is essentially an integration test between CppUTest and
the actual platform environment. To make it fail on Mac OS X, would
require mocking an Ubuntu-style platform. But over time, this mock
would risk going out of sync with the real thing silently, resulting
in the test testing something completely hypothetical, without value
to anyone.

--
Knut Aksel

2012/5/27 Terry Yin <terry....@gmail.com>:

[Knut Aksel Røysland]

Hi Bas.

r736 works like a charm here. Thanks. :-)

I guess there is still a risk that if someone changes the include
order of "MemoryLeakDetectorNewMacros.h" and
"MemoryLeakDetectorMallocMacros.h", or for some reason enables only
the latter, then "StandardCLibrary.h" will be included before or
without "CPPUTEST_USE_STD_CPP_LIB" being set, so my problem would come
back.

Maybe those types of #defines, which are sensitive to include order,
should go to some kind of a "config.h" that any other header file
makes sure to include first when making use of macros defined therein.

However, in this case, one could also stop using
"CPPUTEST_USE_STD_CPP_LIB" and let everyone check on the original
"CPPUTEST_STD_CPP_LIB_DISABLED" instead, thereby reducing the number
of preprocessor variables that risk go out of sync because of
overlapping semantics.

Anyway, I am perfectly happy as it is now for my use, so the above is
just my two cents on what might add a bit of robustness to CppUTest.

--
Thanks,

[Bas Vodde]

Hi Knut,

An old thread, but let me reply it still.

I've created a "config.h"-like file like you suggested, so therefore this shouldn't happen anymore.

Thanks for pointing this out!

Bas