Crash when importing broken dae files

[qarmin]

Godot version:
3.2.4.rc.custom_build. 8385a0d

OS/device including version:
Ubuntu 20.04

Issue description:
When trying to open broken bmp file, then Godot crashes with this backtrace:

ERROR: get: FATAL: Index p_index = 33 is out of bounds (size() = 33).
   At: ./core/cowdata.h:156.
handle_crash: Program crashed with signal 4
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7f330b252210] (??:0)
[2] CowData<float>::get(int) const (/home/rafal/Pulpit/godot3.2/./core/cowdata.h:156 (discriminator 7))
[3] Vector<float>::operator[](int) const (/home/rafal/Pulpit/godot3.2/./core/vector.h:85)
[4] ColladaImport::_create_mesh_surfaces(bool, Ref<ArrayMesh>&, Map<String, Collada::NodeGeometry::Material, Comparator<String>, DefaultAllocator> const&, Collada::MeshData const&, Transform const&, Vector<int> const&, Collada::SkinControllerData const*, Collada::MorphControllerData const*, Vector<Ref<ArrayMesh> >, bool, bool) (/home/rafal/Pulpit/godot3.2/editor/import/editor_import_collada.cpp:775)
[5] ColladaImport::_create_resources(Collada::Node*, bool) (/home/rafal/Pulpit/godot3.2/editor/import/editor_import_collada.cpp:1207 (discriminator 3))
[6] ColladaImport::load(String const&, int, bool, bool) (/home/rafal/Pulpit/godot3.2/editor/import/editor_import_collada.cpp:1292)
[7] EditorSceneImporterCollada::import_scene(String const&, unsigned int, int, List<String, DefaultAllocator>*, Error*) (/home/rafal/Pulpit/godot3.2/editor/import/editor_import_collada.cpp:1776)
[8] ResourceImporterScene::import(String const&, String const&, Map<StringName, Variant, Comparator<StringName>, DefaultAllocator> const&, List<String, DefaultAllocator>*, List<String, DefaultAllocator>*, Variant*) (/home/rafal/Pulpit/godot3.2/editor/import/resource_importer_scene.cpp:1317)
[9] EditorFileSystem::_reimport_file(String const&) (/home/rafal/Pulpit/godot3.2/editor/editor_file_system.cpp:1792)
[10] EditorFileSystem::reimport_files(Vector<String> const&) (/home/rafal/Pulpit/godot3.2/editor/editor_file_system.cpp:1988 (discriminator 3))
[11] EditorFileSystem::_update_scan_actions() (/home/rafal/Pulpit/godot3.2/editor/editor_file_system.cpp:592)
[12] EditorFileSystem::_notification(int) (/home/rafal/Pulpit/godot3.2/editor/editor_file_system.cpp:1168)
[13] EditorFileSystem::_notificationv(int, bool) (/home/rafal/Pulpit/godot3.2/editor/editor_file_system.h:110 (discriminator 14))
[14] Object::notification(int, bool) (/home/rafal/Pulpit/godot3.2/core/object.cpp:931)
[15] SceneTree::_notify_group_pause(StringName const&, int) (/home/rafal/Pulpit/godot3.2/scene/main/scene_tree.cpp:988)
[16] SceneTree::idle(float) (/home/rafal/Pulpit/godot3.2/scene/main/scene_tree.cpp:528 (discriminator 3))
[17] Main::iteration() (/home/rafal/Pulpit/godot3.2/main/main.cpp:2115)
[18] OS_X11::run() (/home/rafal/Pulpit/godot3.2/platform/x11/os_x11.cpp:3628)
[19] godot(main+0x125) [0x16f2a7b] (/home/rafal/Pulpit/godot3.2/platform/x11/godot_x11.cpp:57)
[20] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7f330b2330b3] (??:0)
[21] godot(_start+0x2e) [0x16f289e] (??:?)

Steps to reproduce:

1. Just open minimal project in editor

Minimal reproduction project:
ImportFiles.zip

[gongpha]

ERR_FAIL_INDEX_V function doesn't cover an uv_pos + 1.

godot/editor/import/editor_import_collada.cpp

Lines 712 to 713 in bd42a6c

	ERR_FAIL_INDEX_V(uv_pos, uv_src->array.size(), ERR_INVALID_DATA);
	vertex.uv = Vector3(uv_src->array[uv_pos + 0], 1.0 - uv_src->array[uv_pos + 1], 0);

Is this should have an additional index ?

ERR_FAIL_INDEX_V(uv_pos + 1, uv_src->array.size(), ERR_INVALID_DATA);

[W4RH4WK]

I've looked into this and just adding index checks improves the situation. However, the homegrown XML parser is not robust enough to handle malformed XML. For the provided test inputs, the parser cursor advances beyond the input file causing uninitialized / unmapped memory to be read.

Looking at the parsing logic, there seems to very little room for error. The parser was probably written with the assumption that the input XML is valid. I see the following options:

1. Validate the XML somehow before invoking the current parser, so we only ever feed valid input to the parser. (A Validator is pretty much a parser, so there is probably no benefit here.)
2. Improve the current XML parser to handle malformed inputs gracefully. (If there are specific reasons for keeping the homegrown one around, then this might be the only option.)
3. Replace the current parser with an established, more robust one. (This is my preferred option even though it will probably break the existing API.)

[fire]

How much effort in code and time do you think it would be to get a more robust xml parser?

[W4RH4WK]

I'd say that depends on how robust it should be. Parsers in general are notorious for being error prone. Many security issues originate from feeding malformed input to a parser. While I don't think security is a real concern here, writing your own parser for a given file format is something I simply do not recommend.

As noted in 3, I'd use an established XML parser/library instead. Libxml2 and Expat seem to be regularly used in the Linux world. Some C++ projects seem to rely on RapidXml.

Could one fix/rewrite the parser to be robust enough for our use-cases? Probably. Very rough estimate: ~ 10 hours, probably less.

However, I think this is more of a which approach do we take, in general question. As pretty much the same arguments can be made for other parsers, like modules\bmp\image_loader_bmp.cpp, which crumbles as observed in #46542.

[W4RH4WK]

After thinking about it a bit more, here's a relatively small patch that prevents the parser from simply running off into oblivion. IMHO there is still too little error propagation, but at least it doesn't crash anymore and the API hasn't changed.

[fire]

Godot 4 doesn't crash as of V-Sekai@96410f5 when opening the import project.

[W4RH4WK]

I recommend verifying that the XML parser has been fixed in the meantime and does not read beyond end-of-input on broken XML files. Just because Godot does not crash anymore doesn't mean that everything is handled correctly now.

[fire]

Can you open an issue for verifying broken XML? Not sure how to define all bugs are removed from xml parsing though.