Invalid read in AbcCoreAbstract::getTimeSampling() about alembic

[GoogleCodeExporter]

This error still exists in joetmp:
==17651== Invalid read of size 8
==17651==    at 0x4CB312: 
Alembic::AbcCoreAbstract::v1::TimeSampling::getSampleTime(long) const (in 
/home/jeffs/src_joetmp/alembic_build/lib/Alembic/AbcNice/Tests/AbcNice_PropertyT
ests)
==17651==    by 0x44ECD5: readSimpleProperties(std::string const&) (in 
/home/jeffs/src_joetmp/alembic_build/lib/Alembic/AbcNice/Tests/AbcNice_PropertyT
ests)
==17651==    by 0x451076: main (in 
/home/jeffs/src_joetmp/alembic_build/lib/Alembic/AbcNice/Tests/AbcNice_PropertyT
ests)
==17651==  Address 0x4D28940 is 0 bytes inside a block of size 8 free'd
==17651==    at 0x4A04DAC: operator delete[](void*) (vg_replace_malloc.c:256)
==17651==    by 0x4CCC16: 
boost::detail::sp_counted_impl_pd<Alembic::AbcCoreAbstract::v1::ArraySample*, 
Alembic::AbcCoreAbstract::v1::TArrayDeleter<double> >::dispose() (in 
/home/jeffs/src_joetmp/alembic_build/lib/Alembic/AbcNice/Tests/AbcNice_PropertyT
ests)
==17651==    by 0x4AB1BE: 
Alembic::AbcCoreHDF5::SimplePrImpl<Alembic::AbcCoreAbstract::v1::ScalarPropertyR
eader, Alembic::AbcCoreHDF5::SprImpl, void*>::~SimplePrImpl() (in 
/home/jeffs/src_joetmp/alembic_build/lib/Alembic/AbcNice/Tests/AbcNice_PropertyT
ests)
==17651==    by 0x4BCAE1: Alembic::AbcCoreHDF5::SprImpl::~SprImpl() (in 
/home/jeffs/src_joetmp/alembic_build/lib/Alembic/AbcNice/Tests/AbcNice_PropertyT
ests)
==17651==    by 0x4500C3: readSimpleProperties(std::string const&) (in 
/home/jeffs/src_joetmp/alembic_build/lib/Alembic/AbcNice/Tests/AbcNice_PropertyT
ests)
==17651==    by 0x451076: main (in 
/home/jeffs/src_joetmp/alembic_build/lib/Alembic/AbcNice/Tests/AbcNice_PropertyT
ests)

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

This occurs in AbcNice_PropertyTests, but not the other tests

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

I've confirmed that this bug is still present.  Changing the 
lib/Alembic/AbcNice/Tests/PropertyTests.cpp to have the TimeSamplingType object 
passed into the ODoubleProperty constructor retain time information for 
constant samples made no difference (not that it should have, but you never 
know).  Looking in lib/Alembic/AbcCoreHDF5/SimplePrImpl.h at the 
"getTimeSampling()" method, there's nothing obvious in there.

Here's some more valgrind output, with line numbers and stuff:

==25541== Invalid read of size 8
==25541==    at 0x52B304: 
Alembic::AbcCoreAbstract::v1::TimeSampling::getSampleTime(long) const 
(TimeSampling.cpp:77)
==25541==    by 0x4B5CFF: readSimpleProperties(std::string const&) 
(PropertyTests.cpp:235)
==25541==    by 0x4B773C: main (PropertyTests.cpp:453)
==25541==  Address 0x4D27AD8 is 0 bytes inside a block of size 8 free'd
==25541==    at 0x4A04DAC: operator delete[](void*) (vg_replace_malloc.c:256)
==25541==    by 0x52E02B: 
Alembic::AbcCoreAbstract::v1::TArrayDeleter<double>::operator()(void*) const 
(ArraySample.h:170)
==25541==    by 0x52E070: 
boost::detail::sp_counted_impl_pd<Alembic::AbcCoreAbstract::v1::ArraySample*, 
Alembic::AbcCoreAbstract::v1::TArrayDeleter<double> >::dispose() 
(sp_counted_impl.hpp:144)
==25541==    by 0x4B7D73: boost::detail::sp_counted_base::release() 
(sp_counted_base_gcc_x86.hpp:145)
==25541==    by 0x4B7DCF: boost::detail::shared_count::~shared_count() 
(shared_count.hpp:217)
==25541==    by 0x4E4DF6: 
boost::shared_ptr<Alembic::AbcCoreAbstract::v1::ArraySample>::~shared_ptr() 
(shared_ptr.hpp:169)
==25541==    by 0x5136B9: 
Alembic::AbcCoreHDF5::SimplePrImpl<Alembic::AbcCoreAbstract::v1::ScalarPropertyR
eader, Alembic::AbcCoreHDF5::SprImpl, void*>::~SimplePrImpl() 
(SimplePrImpl.h:428)
==25541==    by 0x51C7F0: Alembic::AbcCoreHDF5::SprImpl::~SprImpl() 
(SprImpl.h:54)
==25541==    by 0x50CACA: void 
boost::checked_delete<Alembic::AbcCoreHDF5::SprImpl>(Alembic::AbcCoreHDF5::SprIm
pl*) (checked_delete.hpp:28)
==25541==    by 0x50D1A0: 
boost::detail::sp_counted_impl_p<Alembic::AbcCoreHDF5::SprImpl>::dispose() 
(sp_counted_impl.hpp:78)
==25541==    by 0x4B7D73: boost::detail::sp_counted_base::release() 
(sp_counted_base_gcc_x86.hpp:145)
==25541==    by 0x4B7DCF: boost::detail::shared_count::~shared_count() 
(shared_count.hpp:217)

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

I'm having trouble grokking this bug... and don't have valgrind (yet).  What is 
the "big picture" view of it? When I do.. it does... 

sorry to be thick - I read these stack traces and it's just confusing.

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

Investigating.

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

OK, I've winnowed down the test to its barest essence.  In the branch 
joesperiment, there is now lib/Alembic/AbcNice/Tests/OctessenceBug17.cpp.  You 
can see the source here:

http://alembic.lucasfilm.com/file/af212c137bb3/lib/Alembic/AbcNice/Tests/Octesse
nceBug17.cpp

This manifests when you get a TimeSampling thingy by doing (and this is a 
really convoluted way to go, and not recommended programming practice):

    const TimeSampling& ts =
            GetCompoundPropertyReaderPtr( c0Props )->
            getScalarProperty( "doubleprop" )->getTimeSampling();

where "c0Props" is an ICompoundProperty.  If you do instead the following, 
which is the recommended way to use the library:

    IDoubleProperty dp( c0Props, "doubleprop" );
    const TimeSampling& ts = dp.getTimeSampling();

then there is no problem.

I'd like to write a test that does the same thing as this, but using only the 
CoreAbstract API, to see if the problem is there, or if there's something going 
on at the Nice level that causes this.

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

One more data point: if you first get a ScalarPropertyReaderPtr, then call 
getTimeSampling(), it works.  Like this:

    ScalarPropertyReaderPtr sprop = c0Props.getPtr()->
        getScalarProperty( "doubleprop" );
    const TimeSampling& ts = sprop->getTimeSampling();

This makes me think it's a bug in AbcCoreAbstract related to initialization of 
the shared pointer.

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

A fix is pending. The problem is incorrect use of shared_ptr.

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

Can you elaborate on, "A fix is pending. The problem is incorrect use of 
shared_ptr".  While I do believe I made a mistake in the ptr usage, since the 
usage of them is so pivotal to the library, I'd like to understand the problem 
in case it is an example of a flawed understanding on my part. That way I can 
check other places to see if I've made similar incorrect understandings.

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

after speaking with Brian, adjusting the time remaining to accurately reflect 
what's left

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

Code changes were fairly small
Ctor for TimeSampling now takes an ArraySamplePtr (this is a boost shared_ptr) 
this way the TimeSampling object will keep it's memory for the timeSamples 
valid as laong as it is still in scope.
lib/Alembic/AbcCoreHDF5/SimplePrImpl.h also changed, because it implement 
getTimeSampling () for all the Property types.
Lastly, udpated our unit tests, specifically a few unit tests manually build a 
free standign TimSampling object (separate from properties).

Note, Joe is gracious helping with integration into the point_9 brnach

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

Hey, Joe, I think you've got this all taken care of, but _if_ it is useful
I've pushed a branch called
octessence17_merged_point_0_9
with merges

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

This is fixed in branch "joemetry".  It was done by making getTimeSampling() 
return a TimeSampling object by value, and having that object contain an 
ArraySamplePtr with the actual time sample information.

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

It's working, hooray!

Original comment by [email protected] on 16 Nov 2010 at 10:59

[GoogleCodeExporter]

All the tests are also now passing on my Mac, double hooray!

Original comment by [email protected] on 16 Nov 2010 at 11:00

[GoogleCodeExporter]

SUH-WEEET!

:)

Original comment by [email protected] on 16 Nov 2010 at 11:00