assimp / assimp

The official Open-Asset-Importer-Library Repository. Loads 40+ 3D-file-formats into one unified and clean data structure.
https://www.assimp.org
Other
10.97k stars 2.91k forks source link

glTF 2.0 importer can abort ReadFile() early #3447

Open sherief opened 4 years ago

sherief commented 4 years ago

Describe the bug glTF 2.0 importer can abort Importer::ReadFile() early, failing to import supported formats.

To Reproduce A call to Importer::ReadFile() is made. In this function is the following snippet:

for( unsigned int a = 0; a < pimpl->mImporter.size(); a++)  {

            if( pimpl->mImporter[a]->CanRead( pFile, pimpl->mIOHandler, false)) {
                imp = pimpl->mImporter[a];
                SetPropertyInteger("importerIndex", a);
                break;
            }
        }

But the glTF 2.0 implementation of CanRead() is problematic as it calls:

asset.Load(pFile, extension == "glb");

And at the beginning of Asset::Load() is

inline void Asset::Load(const std::string &pFile, bool isBinary) {
    ASSIMP_LOG_DEBUG("Loading GLTF2 asset");
    mCurrentAssetDir.clear();
    /*int pos = std::max(int(pFile.rfind('/')), int(pFile.rfind('\\')));
    if (pos != int(std::string::npos)) */

    mCurrentAssetDir = glTFCommon::getCurrentAssetDir(pFile);

    shared_ptr<IOStream> stream(OpenFile(pFile.c_str(), "rb", true));
    if (!stream) {
        throw DeadlyImportError("GLTF: Could not open file for reading");
    }

Note the throw - so a throw from here aborts the loop in Importer::ReadFile() early.

Expected behavior If glTF 2.0 loading fails, Importer::ReadFile() should proceed with trying the following format not exit early.

Screenshots N/A

Desktop (please complete the following information): Windows 10 x64

Additional context Add any other context about the problem here.

sherief commented 4 years ago

Debugging this further, it looks like malformed files also abort the entire load attempt thanks to throwing from AssetMetadata::Read() upon, for example, seeing a mismatched version:

if (version.empty() || version[0] != '2') {
        throw DeadlyImportError("GLTF: Unsupported glTF version: ", version);
    }

This should be more graceful - CanRead() should return false, not throw an exception and unwind the callstack.