dartsim / dart

DART: Dynamic Animation and Robotics Toolkit
http://dartsim.github.io/
BSD 2-Clause "Simplified" License
892 stars 285 forks source link

Memory leaks in reading meshes using Assimp #565

Open jslee02 opened 8 years ago

jslee02 commented 8 years ago

Memory leaks in reading meshes was reported by @sehoonha. Here is the test code:

TEST(SdfParser, SdfParserMemoryLeak)
{
  // Regression test
  for (int i = 0; i < 18; i++)
  {
    SkeletonPtr temp = SoftSdfParser::readSkeleton(
          DART_DATA_PATH"sdf/atlas/atlas_v3_no_head_soft_feet.sdf");
    if (!temp)
    {
      std::cout << "Load " << i << "-th atlas NG" << std::endl;
      exit(0);
    } else
    {
      std::cout << "Load " << i << "-th atlas OK" << std::endl;
    }
  }
}

Once it reached the 18-th iteration, it fails to allocate memory.

The main cause was that LocalResource instance is not released after it's created. Simply, AssimpInputResourceRetrieverAdaptor::Open() is called when Assimp opens a file but Assimp doesn't call AssimpInputResourceRetrieverAdaptor::Close() when the file needs to be closed. Instead, it just destructs the AssimpInputResourceRetrieverAdaptor instance.

This is reported to the upstream.

I quickly tested with my patch, that is described in the report, for two mesh types (stl, dae), and it worked. But I would like to wait for feedbacks from the community before create a pull request to see if it's the correct approach. It seems this patch should be applied to other mesh types (more than 40!) so I don't want to start it without assurance.

kimkulling commented 8 years ago

Is this issue only related to Collada files? Because in assimp-master I just fixed this leak.

Kimmi

jslee02 commented 8 years ago

Thank you for the real quick fix!

I observed that it also happens in STLImporter::InternReadFile(). Other importers are not tested, but I think it would probably happen in other importers that call IOSystem::Open() but don't call `IOSystem::Close() after file import.

Here is the search result of the calls of IOSystem::Open() and IOSystem::Close():

js@js:~/dev/assimp$ grep -rnw '.' -e "pIOHandler->Open"
./doc/dox.h:1683:   boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/3DSLoader.cpp:158:    StreamReaderLE stream(pIOHandler->Open(pFile,"rb"));
./code/BaseImporter.cpp:152:    boost::scoped_ptr<IOStream> pStream (pIOHandler->Open(pFile));
./code/BaseImporter.cpp:259:    boost::scoped_ptr<IOStream> pStream (pIOHandler->Open(pFile));
./code/COBLoader.cpp:147:    boost::scoped_ptr<StreamReaderLE> stream(new StreamReaderLE( pIOHandler->Open(pFile,"rb")) );
./code/RawLoader.cpp:102:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/BlenderLoader.cpp:166:    boost::shared_ptr<IOStream> stream(pIOHandler->Open(pFile,"rb"));
./code/DXFLoader.cpp:137:    boost::shared_ptr<IOStream> file = boost::shared_ptr<IOStream>( pIOHandler->Open( pFile) );
./code/TerragenLoader.cpp:123:    IOStream* file = pIOHandler->Open( pFile, "rb");
./code/OgreMaterial.cpp:172:            materialFile = pIOHandler->Open(potentialFiles[i]);
./code/glTFImporter.cpp:105:        boost::scoped_ptr<IOStream> pStream(pIOHandler->Open(pFile));
./code/Q3DLoader.cpp:110:    StreamReaderLE stream(pIOHandler->Open(pFile,"rb"));
./code/ASELoader.cpp:133:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/HMPLoader.cpp:117:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/UnrealLoader.cpp:158:    IOStream* p = pIOHandler->Open(d_path);
./code/UnrealLoader.cpp:161:    StreamReaderLE d_reader(pIOHandler->Open(d_path));
./code/UnrealLoader.cpp:206:    p = pIOHandler->Open(a_path);
./code/UnrealLoader.cpp:209:    StreamReaderLE a_reader(pIOHandler->Open(a_path));
./code/UnrealLoader.cpp:238:    boost::scoped_ptr<IOStream> pb (pIOHandler->Open(uc_path));
./code/MS3DLoader.cpp:216:    StreamReaderLE stream(pIOHandler->Open(pFile,"rb"));
./code/MD5Loader.cpp:357:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/MD5Loader.cpp:572:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/MD5Loader.cpp:684:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/FBXImporter.cpp:144:    boost::scoped_ptr<IOStream> stream(pIOHandler->Open(pFile,"rb"));
./code/MD3Loader.cpp:740:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/MD2Loader.cpp:197:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/OFFLoader.cpp:112:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/STLLoader.cpp:175:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/NDOLoader.cpp:117:    StreamReaderBE reader(pIOHandler->Open( pFile, "rb"));
./code/LWSLoader.cpp:514:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/ColladaParser.cpp:91:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile ) );
./code/BVHLoader.cpp:121:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/XFileImporter.cpp:114:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/SMDLoader.cpp:125:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/NFFLoader.cpp:136:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( path, "rb"));
./code/NFFLoader.cpp:238:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/irrXMLWrapper.h:59: * boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/MDCLoader.cpp:222:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/OgreXmlSerializer.cpp:745:    boost::scoped_ptr<IOStream> file(pIOHandler->Open(filename));
./code/IRRLoader.cpp:905:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/AssbinLoader.cpp:87:    IOStream * in = pIOHandler->Open(pFile);
./code/AssbinLoader.cpp:639:    IOStream * stream = pIOHandler->Open(pFile,"rb");
./code/OgreImporter.cpp:101:    IOStream *f = pIOHandler->Open(pFile, "rb");
./code/OgreBinarySerializer.cpp:905:    IOStream *f = pIOHandler->Open(filename, "rb");
./code/OpenGEXImporter.cpp:248:    IOStream *file = pIOHandler->Open( filename, "rb" );
./code/MDLLoader.cpp:158:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/IRRMeshLoader.cpp:124:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/LWOLoader.cpp:142:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/XGLLoader.cpp:156:    boost::shared_ptr<IOStream> stream( pIOHandler->Open( pFile, "rb"));
./code/CSMLoader.cpp:125:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/ObjFileImporter.cpp:121:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, mode));
./code/ACLoader.cpp:790:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
./code/PlyLoader.cpp:130:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/B3DImporter.cpp:116:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/C4DImporter.cpp:134:    boost::scoped_ptr<IOStream> file( pIOHandler->Open( pFile));
./code/MDLMaterialLoader.cpp:66:    IOStream* pcStream = pIOHandler->Open(configPalette,"rb");
./code/IFCLoader.cpp:171:    boost::shared_ptr<IOStream> stream(pIOHandler->Open(pFile));

js@js:~/dev/assimp$ grep -rnw '.' -e "pIOHandler->Close"
./code/ColladaParser.cpp:107:    pIOHandler->Close( file.get() );
./code/AssbinLoader.cpp:94:    pIOHandler->Close(in);
./code/AssbinLoader.cpp:684:    pIOHandler->Close(stream);

I can't say the searched numbers of pIOHandler->Open and pIOHandler->Close should be equal, but I think it would worth to check.

stale[bot] commented 6 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.