[Boost.Test] valgrind complains about invalid reads
Boost 1.37.0, Linux, x86_64, gcc 4.1: Valgrind 3.3.1 complains when running this simple test: #define BOOST_TEST_MAIN #include <boost/test/unit_test.hpp> BOOST_AUTO_TEST_CASE( test1 ) { BOOST_CHECK( 1 == 1 ); } The valgrind messages: ==1836== Memcheck, a memory error detector. ==1836== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al. ==1836== Using LibVEX rev 1854, a library for dynamic binary translation. ==1836== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP. ==1836== Using valgrind-3.3.1, a dynamic binary instrumentation framework. ==1836== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al. ==1836== For more details, rerun with: -v ==1836== Running 1 test case... *** No errors detected ==1836== Invalid read of size 8 ==1836== at 0x41E41C: __tcf_1 (in RegressionTest) ==1836== by 0x3830632FA4: exit (in /lib64/libc-2.5.so) ==1836== by 0x383061D8BA: (below main) (in /lib64/libc-2.5.so) ==1836== Address 0x4c1ade0 is 40 bytes inside a block of size 48 free'd ==1836== at 0x4A0555C: operator delete(void*) (vg_replace_malloc.c:342) ==1836== by 0x42024C: std::_Rb_tree<unsigned long, std::pair<unsigned long const, boost::unit_test::test_unit*>, std::_Select1st<std::pair<unsigned long const, boost::unit_test::test_unit*> >, std::less<unsigned long>, std::allocator<std::pair<unsigned long const, boost::unit_test::test_unit*> > >::erase(unsigned long const&) (in RegressionTest) ==1836== by 0x417C11: boost::unit_test::test_unit::~test_unit() (in RegressionTest) ==1836== by 0x41E41B: __tcf_1 (in RegressionTest) ==1836== by 0x3830632FA4: exit (in /lib64/libc-2.5.so) ==1836== by 0x383061D8BA: (below main) (in /lib64/libc-2.5.so) ==1836== ==1836== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 5 from 1) ==1836== malloc/free: in use at exit: 0 bytes in 0 blocks. ==1836== malloc/free: 89 allocs, 89 frees, 38,935 bytes allocated. ==1836== For counts of detected errors, rerun with: -v ==1836== All heap blocks were freed -- no leaks are possible. I was not able to figure out what valgrind is really complaining about. The internal map of test units holds two entries. One corresponds to my BOOST_AUTO_TEST_CASE call (id=1), the other one seems to be used internally (id=65536). Both are properly erased in deregister_test_unit() which is called by ~test_unit(). Maybe someone with a little bit more insight into Boost.Test is willing to look at this. Regards, Peter.
Try compiling a debug version. That will give you line numbers to help you track it down easier. James On Nov 14, 2008, at 6:04 AM, Peter Klotz wrote:
Boost 1.37.0, Linux, x86_64, gcc 4.1:
Valgrind 3.3.1 complains when running this simple test:
#define BOOST_TEST_MAIN #include <boost/test/unit_test.hpp>
BOOST_AUTO_TEST_CASE( test1 ) { BOOST_CHECK( 1 == 1 ); }
The valgrind messages:
==1836== Memcheck, a memory error detector. ==1836== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al. ==1836== Using LibVEX rev 1854, a library for dynamic binary translation. ==1836== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP. ==1836== Using valgrind-3.3.1, a dynamic binary instrumentation framework. ==1836== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al. ==1836== For more details, rerun with: -v ==1836== Running 1 test case...
*** No errors detected ==1836== Invalid read of size 8 ==1836== at 0x41E41C: __tcf_1 (in RegressionTest) ==1836== by 0x3830632FA4: exit (in /lib64/libc-2.5.so) ==1836== by 0x383061D8BA: (below main) (in /lib64/libc-2.5.so) ==1836== Address 0x4c1ade0 is 40 bytes inside a block of size 48 free'd ==1836== at 0x4A0555C: operator delete(void*) (vg_replace_malloc.c:342) ==1836== by 0x42024C: std::_Rb_tree<unsigned long, std::pair<unsigned long const, boost::unit_test::test_unit*>, std::_Select1st<std::pair<unsigned long const, boost::unit_test::test_unit*> >, std::less<unsigned long>, std::allocator<std::pair<unsigned long const, boost::unit_test::test_unit*> > >::erase(unsigned long const&) (in RegressionTest) ==1836== by 0x417C11: boost::unit_test::test_unit::~test_unit() (in RegressionTest) ==1836== by 0x41E41B: __tcf_1 (in RegressionTest) ==1836== by 0x3830632FA4: exit (in /lib64/libc-2.5.so) ==1836== by 0x383061D8BA: (below main) (in /lib64/libc-2.5.so) ==1836== ==1836== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 5 from 1) ==1836== malloc/free: in use at exit: 0 bytes in 0 blocks. ==1836== malloc/free: 89 allocs, 89 frees, 38,935 bytes allocated. ==1836== For counts of detected errors, rerun with: -v ==1836== All heap blocks were freed -- no leaks are possible.
I was not able to figure out what valgrind is really complaining about.
The internal map of test units holds two entries. One corresponds to my BOOST_AUTO_TEST_CASE call (id=1), the other one seems to be used internally (id=65536).
Both are properly erased in deregister_test_unit() which is called by ~test_unit().
Maybe someone with a little bit more insight into Boost.Test is willing to look at this.
Regards, Peter. _______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
James Sutherland wrote:
Try compiling a debug version. That will give you line numbers to help you track it down easier. James
Found the problem. It was a non virtual destructor in class test_unit. Since test_unit is the base class of test_case, test_suite and master_test_suite_t it needs a virtual destructor. In deregister_test_unit() a test_unit* pointer is passed which may actually point to a derived class. Calling delete without a virtual destructor leads to undefined behavior (and makes valgrind complain). The attached patch against 1.37.0 fixes this problem. Regards, Peter.
Peter Klotz wrote:
James Sutherland wrote:
Try compiling a debug version. That will give you line numbers to help you track it down easier. James
Found the problem. It was a non virtual destructor in class test_unit.
Since test_unit is the base class of test_case, test_suite and master_test_suite_t it needs a virtual destructor.
No. It does not. We never delete test units through pointer to test_unit.
In deregister_test_unit() a test_unit* pointer is passed which may actually point to a derived class. Calling delete without a virtual destructor leads to undefined behavior (and makes valgrind complain).
deregister_test_unit does not cause delete command. In fact it's being call from destructor. Gennadiy
Gennadiy Rozental wrote:
Peter Klotz wrote:
James Sutherland wrote:
Try compiling a debug version. That will give you line numbers to help you track it down easier. James
Found the problem. It was a non virtual destructor in class test_unit.
Since test_unit is the base class of test_case, test_suite and master_test_suite_t it needs a virtual destructor.
No. It does not. We never delete test units through pointer to test_unit.
In deregister_test_unit() a test_unit* pointer is passed which may actually point to a derived class. Calling delete without a virtual destructor leads to undefined behavior (and makes valgrind complain).
deregister_test_unit does not cause delete command. In fact it's being call from destructor.
Please see the output of my simple example (with extra output in Boost.Test): With virtual destructor in test_unit: register_test_unit( test_case* tc ) tc=0x42c4ab0 register_test_unit( test_suite* ts ) ts=0x42c4ba8 Running 1 test case... *** No errors detected deregister_test_unit( test_unit* tu ) tu=0x42c4ba8 deregister_test_unit( test_unit* tu ) tu=0x42c4ab0 Here the pointers in register_test_unit()/deregister_test_unit() match as expected. Without virtual destructor in test_unit:register_test_unit( test_case* tc ) register_test_unit( test_case* tc ) tc=0x42c4ab0 register_test_unit( test_suite* ts ) ts=0x42c4ba8 Running 1 test case... *** No errors detected deregister_test_unit( test_unit* tu ) tu=0x42c4bac deregister_test_unit( test_unit* tu ) tu=0x42c4ab0 Here the test_unit pointer is not correct and does not match the one initially registered! I agree that deregister_test_unit() does not delete the test_suite but the pointers ought to match. Maybe you can run my simple test case using valgrind. I used version 3.3.1 under Ubuntu 8.10 i386 (gcc 4.3.2) and Red Hat Enterprise Linux 5 x86_64 (gcc 4.1.2). Regards, Peter.
Peter Klotz <peter.klotz <at> aon.at> writes:
Here the test_unit pointer is not correct and does not match the one initially registered!
So what? Id match, right? test_unit* is shifted by 4 bytes from test_suite* (it does have virtual destructor). In fact I do not see a reason, why test_suite need virtual destructor. Maybe if I remove it, valgrind will be happy? Though I still believe code is valid as is.
I agree that deregister_test_unit() does not delete the test_suite but the pointers ought to match.
No they should not.
Maybe you can run my simple test case using valgrind. I used version 3.3.1 under Ubuntu 8.10 i386 (gcc 4.3.2) and Red Hat Enterprise Linux 5 x86_64 (gcc 4.1.2).
I do not have valgrind easily accessible at home.
Gennadiy Rozental wrote:
Peter Klotz <peter.klotz <at> aon.at> writes:
Here the test_unit pointer is not correct and does not match the one initially registered!
So what? Id match, right? test_unit* is shifted by 4 bytes from test_suite* (it does have virtual destructor).
In fact I do not see a reason, why test_suite need virtual destructor. Maybe if I remove it, valgrind will be happy? Though I still believe code is valid as is.
I agree that deregister_test_unit() does not delete the test_suite but the pointers ought to match.
No they should not.
Maybe you can run my simple test case using valgrind. I used version 3.3.1 under Ubuntu 8.10 i386 (gcc 4.3.2) and Red Hat Enterprise Linux 5 x86_64 (gcc 4.1.2).
I do not have valgrind easily accessible at home.
OK, simple solution then. Why not just adding the virtual destructor to make valgrind happy? No one will ever report this issue again and our several hundred internal tests will show no false leaks either. Regards, Peter.
Gennadiy Rozental wrote:
Peter Klotz <peter.klotz <at> aon.at> writes:
Why not just adding the virtual destructor to make valgrind happy?
Did you try removing virtual from test_suite destructor instead?
Yes, just making ~test_suite() non virtual worsens things. valgrind complains about twice as many "invalid read" errors. Here my results: Boost 1.37.0 unpatched (~test_suite() is virtual, ~test_unit() is not): ==22485== Invalid read of size 4 ... ==22485== by 0x806BB09: boost::unit_test::framework::deregister_test_unit(boost::unit_test::test_unit*) (framework.ipp:326) ==22485== by 0x80602EA: boost::unit_test::test_unit::~test_unit() (unit_test_suite.ipp:65) ==22485== by 0x806F06C: boost::unit_test::test_case::~test_case() (unit_test_suite_impl.hpp:110) Boost 1.37.0 patched (~test_suite() and ~test_unit() are non virtual): ==22946== Invalid read of size 4 ... ==22946== by 0x806B9E5: boost::unit_test::framework::deregister_test_unit(boost::unit_test::test_unit*) (framework.ipp:326) ==22946== by 0x80602E2: boost::unit_test::test_unit::~test_unit() (unit_test_suite.ipp:65) ==22946== by 0x806F236: boost::unit_test::test_case::~test_case() (unit_test_suite_impl.hpp:110) ==22946== Invalid read of size 4 ... ==22946== by 0x806B9E5: boost::unit_test::framework::deregister_test_unit(boost::unit_test::test_unit*) (framework.ipp:326) ==22946== by 0x80602E2: boost::unit_test::test_unit::~test_unit() (unit_test_suite.ipp:65) ==22946== by 0x806F214: boost::unit_test::test_suite::~test_suite() (unit_test_suite_impl.hpp:141) ==22946== by 0x806F2A7: boost::unit_test::framework_impl::clear() (framework.ipp:131) Making ~test_unit() virtual removes all errors. Then even a non virtual ~test_suite() is no longer a problem. So the final conclusion would be to remove virtual from ~test_suite() and add it to the base class destructor ~test_unit(). Regards, Peter.
Peter Klotz wrote:
Gennadiy Rozental wrote:
Peter Klotz <peter.klotz <at> aon.at> writes:
Why not just adding the virtual destructor to make valgrind happy?
Did you try removing virtual from test_suite destructor instead?
Yes, just making ~test_suite() non virtual worsens things.
valgrind complains about twice as many "invalid read" errors.
[...]
Making ~test_unit() virtual removes all errors. Then even a non virtual ~test_suite() is no longer a problem.
So the final conclusion would be to remove virtual from ~test_suite() and add it to the base class destructor ~test_unit().
Peter, I am not convinced there is a bug inside the Boost.Test code. As far as I can tell all I am doing is legitimate. Maybe we should report this to the valgrind developers instead? Gennadiy
On 19 Dec 2008, at 07:23, Gennadiy Rozental wrote:
Peter Klotz wrote:
Gennadiy Rozental wrote:
Peter Klotz <peter.klotz <at> aon.at> writes:
Why not just adding the virtual destructor to make valgrind happy?
Did you try removing virtual from test_suite destructor instead?
Yes, just making ~test_suite() non virtual worsens things. valgrind complains about twice as many "invalid read" errors.
[...]
Making ~test_unit() virtual removes all errors. Then even a non virtual ~test_suite() is no longer a problem. So the final conclusion would be to remove virtual from ~test_suite() and add it to the base class destructor ~test_unit().
Peter,
I am not convinced there is a bug inside the Boost.Test code. As far as I can tell all I am doing is legitimate. Maybe we should report this to the valgrind developers instead?
Can you produce a reduced test case, without any boost headers? I'd be happy to have a look at it. Chris
Gennadiy Rozental wrote:
Peter Klotz wrote:
Gennadiy Rozental wrote:
Peter Klotz <peter.klotz <at> aon.at> writes:
Why not just adding the virtual destructor to make valgrind happy?
Did you try removing virtual from test_suite destructor instead?
Yes, just making ~test_suite() non virtual worsens things.
valgrind complains about twice as many "invalid read" errors.
[...]
Making ~test_unit() virtual removes all errors. Then even a non virtual ~test_suite() is no longer a problem.
So the final conclusion would be to remove virtual from ~test_suite() and add it to the base class destructor ~test_unit().
Peter,
I am not convinced there is a bug inside the Boost.Test code. As far as I can tell all I am doing is legitimate. Maybe we should report this to the valgrind developers instead?
Hello Gennadiy I finally found the time to extract the code portion from Boost.Test that shows the valgrind error. Meanwhile I have switched to Boost 1.39.0, gcc 4.3.3 and valgrind 3.4.1. The attached sample is a standalone program and can be compiled like this: g++ -g -W -Wall BoostTestValgrindError.cpp -o BoostTestValgrindError Running it with valgrind (valgrind ./BoostTestValgrindError) shows the error. I stripped everything that is not necessary to reproduce the problem. The internal map of test cases and test suites is replaced by a set. The reason for the error seems to be the downcast in your clear() method in framework_impl. Assigning the downcasted pointer to a variable and then performing the delete resolves the problem. There seems to be a subtle difference between these two alternatives. My suggestion that would also clean up the clear() method is the following: * Add the virtual destructor to class test_unit * Rewrite framework_impl::clear() this way: void clear() { while( !m_test_units.empty() ) delete m_test_units.begin()->second; // the delete will erase this element from the map } The virtual destructor ensures that you can safely destroy your derived classes through the base class pointer stored in m_test_units. The distinction between test suites and test cases in clear() is then no longer needed and the downcast is also gone. Regards, Peter.
participants (5)
-
Christopher Jefferson -
Gennadiy Rozental -
James Sutherland -
Peter Klotz -
Peter Klotz