[fbx] fix crash in FBX parser caused by mesh geometry #44371

RevoluPowered · 2020-12-14T17:39:35Z

Fixed file crash in #44371 caused by invalid index, will now only run when the index is valid.
Fixed buffer overflow in #44376 these assets

Bugsquad edit: Fixes #43949, fixes #44376.

qarmin · 2020-12-14T18:41:06Z

I still have buffer overflow in this PR

==336498==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6300071bfaa5 at pc 0x000003ece720 bp 0x7ffc4c302230 sp 0x7ffc4c302220
READ of size 1 at 0x6300071bfaa5 thread T0
    #0 0x3ece71f in FBXDocParser::Tokenize(std::vector<FBXDocParser::Token*, std::allocator<FBXDocParser::Token*> >&, char const*) modules/fbx/fbx_parser/FBXTokenizer.cpp:155
    #1 0x2c98395 in EditorSceneImporterFBX::import_scene(String const&, unsigned int, int, List<String, DefaultAllocator>*, Error*) modules/fbx/editor_scene_importer_fbx.cpp:131
    #2 0x7099000 in ResourceImporterScene::import(String const&, String const&, Map<StringName, Variant, Comparator<StringName>, DefaultAllocator> const&, List<String, DefaultAllocator>*, List<String, DefaultAllocator>*, Variant*) editor/import/resource_importer_scene.cpp:1320
    #3 0x5b7b268 in EditorFileSystem::_reimport_file(String const&) editor/editor_file_system.cpp:1798
    #4 0x5b86860 in EditorFileSystem::reimport_files(Vector<String> const&) editor/editor_file_system.cpp:1996
    #5 0x5b43586 in EditorFileSystem::_update_scan_actions() editor/editor_file_system.cpp:589
    #6 0x5b5d347 in EditorFileSystem::_notification(int) editor/editor_file_system.cpp:1174
    #7 0x5b969af in EditorFileSystem::_notificationv(int, bool) (/home/rafal/Pobrane/godot-fix-parser-crash-mesh-geometry/bin/godot.x11.tools.64s+0x5b969af)
    #8 0xe7f529b in Object::notification(int, bool) core/object.cpp:929
    #9 0x95e712d in SceneTree::_notify_group_pause(StringName const&, int) scene/main/scene_tree.cpp:985
    #10 0x95d79ef in SceneTree::idle(float) scene/main/scene_tree.cpp:525
    #11 0x1542726 in Main::iteration() main/main.cpp:2111
    #12 0x1434eb0 in OS_X11::run() platform/x11/os_x11.cpp:3608
    #13 0x13a2b56 in main platform/x11/godot_x11.cpp:56
    #14 0x7ff1592370b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #15 0x13a276d in _start (/home/rafal/Pobrane/godot-fix-parser-crash-mesh-geometry/bin/godot.x11.tools.64s+0x13a276d)

0x6300071bfaa5 is located 0 bytes to the right of 63141-byte region [0x6300071b0400,0x6300071bfaa5)
allocated by thread T0 here:
    #0 0x7ff15a6fc517 in malloc (/lib/x86_64-linux-gnu/libasan.so.6+0xb0517)
    #1 0xed2f31d in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:82
    #2 0x161b31a in PoolVector<unsigned char>::resize(int) core/pool_vector.h:582
    #3 0x2c97070 in EditorSceneImporterFBX::import_scene(String const&, unsigned int, int, List<String, DefaultAllocator>*, Error*) modules/fbx/editor_scene_importer_fbx.cpp:107
    #4 0x7099000 in ResourceImporterScene::import(String const&, String const&, Map<StringName, Variant, Comparator<StringName>, DefaultAllocator> const&, List<String, DefaultAllocator>*, List<String, DefaultAllocator>*, Variant*) editor/import/resource_importer_scene.cpp:1320
    #5 0x5b7b268 in EditorFileSystem::_reimport_file(String const&) editor/editor_file_system.cpp:1798
    #6 0x5b86860 in EditorFileSystem::reimport_files(Vector<String> const&) editor/editor_file_system.cpp:1996
    #7 0x5b43586 in EditorFileSystem::_update_scan_actions() editor/editor_file_system.cpp:589
    #8 0x5b5d347 in EditorFileSystem::_notification(int) editor/editor_file_system.cpp:1174
    #9 0x5b969af in EditorFileSystem::_notificationv(int, bool) (/home/rafal/Pobrane/godot-fix-parser-crash-mesh-geometry/bin/godot.x11.tools.64s+0x5b969af)
    #10 0xe7f529b in Object::notification(int, bool) core/object.cpp:929
    #11 0x95e712d in SceneTree::_notify_group_pause(StringName const&, int) scene/main/scene_tree.cpp:985
    #12 0x95d79ef in SceneTree::idle(float) scene/main/scene_tree.cpp:525
    #13 0x1542726 in Main::iteration() main/main.cpp:2111
    #14 0x1434eb0 in OS_X11::run() platform/x11/os_x11.cpp:3608
    #15 0x13a2b56 in main platform/x11/godot_x11.cpp:56
    #16 0x7ff1592370b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

RevoluPowered · 2020-12-14T20:03:13Z

I still have buffer overflow in this PR

==336498==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6300071bfaa5 at pc 0x000003ece720 bp 0x7ffc4c302230 sp 0x7ffc4c302220
READ of size 1 at 0x6300071bfaa5 thread T0
    #0 0x3ece71f in FBXDocParser::Tokenize(std::vector<FBXDocParser::Token*, std::allocator<FBXDocParser::Token*> >&, char const*) modules/fbx/fbx_parser/FBXTokenizer.cpp:155
    #1 0x2c98395 in EditorSceneImporterFBX::import_scene(String const&, unsigned int, int, List<String, DefaultAllocator>*, Error*) modules/fbx/editor_scene_importer_fbx.cpp:131
    #2 0x7099000 in ResourceImporterScene::import(String const&, String const&, Map<StringName, Variant, Comparator<StringName>, DefaultAllocator> const&, List<String, DefaultAllocator>*, List<String, DefaultAllocator>*, Variant*) editor/import/resource_importer_scene.cpp:1320
    #3 0x5b7b268 in EditorFileSystem::_reimport_file(String const&) editor/editor_file_system.cpp:1798
    #4 0x5b86860 in EditorFileSystem::reimport_files(Vector<String> const&) editor/editor_file_system.cpp:1996
    #5 0x5b43586 in EditorFileSystem::_update_scan_actions() editor/editor_file_system.cpp:589
    #6 0x5b5d347 in EditorFileSystem::_notification(int) editor/editor_file_system.cpp:1174
    #7 0x5b969af in EditorFileSystem::_notificationv(int, bool) (/home/rafal/Pobrane/godot-fix-parser-crash-mesh-geometry/bin/godot.x11.tools.64s+0x5b969af)
    #8 0xe7f529b in Object::notification(int, bool) core/object.cpp:929
    #9 0x95e712d in SceneTree::_notify_group_pause(StringName const&, int) scene/main/scene_tree.cpp:985
    #10 0x95d79ef in SceneTree::idle(float) scene/main/scene_tree.cpp:525
    #11 0x1542726 in Main::iteration() main/main.cpp:2111
    #12 0x1434eb0 in OS_X11::run() platform/x11/os_x11.cpp:3608
    #13 0x13a2b56 in main platform/x11/godot_x11.cpp:56
    #14 0x7ff1592370b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #15 0x13a276d in _start (/home/rafal/Pobrane/godot-fix-parser-crash-mesh-geometry/bin/godot.x11.tools.64s+0x13a276d)

0x6300071bfaa5 is located 0 bytes to the right of 63141-byte region [0x6300071b0400,0x6300071bfaa5)
allocated by thread T0 here:
    #0 0x7ff15a6fc517 in malloc (/lib/x86_64-linux-gnu/libasan.so.6+0xb0517)
    #1 0xed2f31d in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:82
    #2 0x161b31a in PoolVector<unsigned char>::resize(int) core/pool_vector.h:582
    #3 0x2c97070 in EditorSceneImporterFBX::import_scene(String const&, unsigned int, int, List<String, DefaultAllocator>*, Error*) modules/fbx/editor_scene_importer_fbx.cpp:107
    #4 0x7099000 in ResourceImporterScene::import(String const&, String const&, Map<StringName, Variant, Comparator<StringName>, DefaultAllocator> const&, List<String, DefaultAllocator>*, List<String, DefaultAllocator>*, Variant*) editor/import/resource_importer_scene.cpp:1320
    #5 0x5b7b268 in EditorFileSystem::_reimport_file(String const&) editor/editor_file_system.cpp:1798
    #6 0x5b86860 in EditorFileSystem::reimport_files(Vector<String> const&) editor/editor_file_system.cpp:1996
    #7 0x5b43586 in EditorFileSystem::_update_scan_actions() editor/editor_file_system.cpp:589
    #8 0x5b5d347 in EditorFileSystem::_notification(int) editor/editor_file_system.cpp:1174
    #9 0x5b969af in EditorFileSystem::_notificationv(int, bool) (/home/rafal/Pobrane/godot-fix-parser-crash-mesh-geometry/bin/godot.x11.tools.64s+0x5b969af)
    #10 0xe7f529b in Object::notification(int, bool) core/object.cpp:929
    #11 0x95e712d in SceneTree::_notify_group_pause(StringName const&, int) scene/main/scene_tree.cpp:985
    #12 0x95d79ef in SceneTree::idle(float) scene/main/scene_tree.cpp:525
    #13 0x1542726 in Main::iteration() main/main.cpp:2111
    #14 0x1434eb0 in OS_X11::run() platform/x11/os_x11.cpp:3608
    #15 0x13a2b56 in main platform/x11/godot_x11.cpp:56
    #16 0x7ff1592370b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

is this happening on the 3.2 branch too?

qarmin · 2020-12-14T20:07:00Z

Yes, buffer overflow happens both in this PR and 3.2 Godot branch
Project from issue which I use - https://github.com/godotengine/godot/files/5611825/Godot.issue.zip

RevoluPowered · 2020-12-14T20:09:06Z

OK, what I propose we do instead is merge this fix for now, and make it a separate issue. One is with the loading of the FBX and the other is in another area, is that OK?

also, I forget what options do you use again use_asan=yes, use_lsan=yes, am i missing any flags?

qarmin · 2020-12-14T20:30:50Z

I thought that since this buffer overflow occurs at the same scene that causes Godot crash(from linked issue), these bugs can be related.
If it is something else that causes an error, it is even better to create a separate PR.

Yup, use_asan=yes, use_lsan=yes are only flags which I'm using to get this log.

RevoluPowered · 2020-12-14T21:09:48Z

I thought that since this buffer overflow occurs at the same scene that causes Godot crash(from linked issue), these bugs can be related.
If it is something else that causes an error, it is even better to create a separate PR.

Yup, use_asan=yes, use_lsan=yes are only flags which I'm using to get this log.

yeah I think this is a bug with all ASCII files for the FBXTokenizer

for (const char *cur = input; *cur; column += (*cur == '\t' ? ASSIMP_FBX_TAB_WIDTH : 1), ++cur) {

This code to me is quite bad so I'll try and constrain it correctly.

To make this safer I've done this:

	for( size_t x = 0; x < strlen(input); x++)
	{
		const char c = input[x];
		const char *cur = &input[x];
		column += (c == '\t' ? ASSIMP_FBX_TAB_WIDTH : 1);
        }

RevoluPowered · 2020-12-14T21:54:21Z

OK now it is :) I force pushed 👯‍♂️ 👯‍♀️

…verflow Fixes: - Element collection will only contain valid elements. - Fixes buffer overflow in the FBX document

akien-mga · 2020-12-15T12:04:12Z

Thanks!

RevoluPowered force-pushed the fix-parser-crash-mesh-geometry branch from 14fe46e to 53460a9 Compare December 14, 2020 17:40

RevoluPowered changed the title ~~Fix parser crash mesh geometry~~ [fbx] fix crash in FBX parser caused by mesh geometry Dec 14, 2020

RevoluPowered requested a review from akien-mga December 14, 2020 17:41

RevoluPowered force-pushed the fix-parser-crash-mesh-geometry branch from 53460a9 to cad3fb8 Compare December 14, 2020 18:10

RevoluPowered mentioned this pull request Dec 14, 2020

[FBX] buffer overflow on loading FBX file #44376

Closed

RevoluPowered force-pushed the fix-parser-crash-mesh-geometry branch from 0ef6c8d to 4838f3a Compare December 14, 2020 21:53

[fbx] Fix godotengine#44371 godotengine#44376 File crash and Buffer O…

18d1898

…verflow Fixes: - Element collection will only contain valid elements. - Fixes buffer overflow in the FBX document

RevoluPowered force-pushed the fix-parser-crash-mesh-geometry branch from 4838f3a to 18d1898 Compare December 14, 2020 21:57

RevoluPowered added bug crash topic:import labels Dec 14, 2020

akien-mga added this to the 4.0 milestone Dec 15, 2020

akien-mga approved these changes Dec 15, 2020

View reviewed changes

akien-mga merged commit 6956b01 into godotengine:3.2 Dec 15, 2020

akien-mga modified the milestones: 4.0, 3.2 Dec 15, 2020

akien-mga mentioned this pull request Dec 15, 2020

[FBX] crashing due to parser error 'GetRequiredElement' is not gracefully failing #43949

Closed

akien-mga modified the milestones: 3.2, 3.3 Apr 20, 2021

yjh0502 mentioned this pull request Aug 16, 2021

fixes #10946, auto-reload with external editor #51724

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fbx] fix crash in FBX parser caused by mesh geometry #44371

[fbx] fix crash in FBX parser caused by mesh geometry #44371

RevoluPowered commented Dec 14, 2020 •

edited by akien-mga

Loading

qarmin commented Dec 14, 2020

RevoluPowered commented Dec 14, 2020

qarmin commented Dec 14, 2020

RevoluPowered commented Dec 14, 2020 •

edited

Loading

qarmin commented Dec 14, 2020

RevoluPowered commented Dec 14, 2020 •

edited

Loading

RevoluPowered commented Dec 14, 2020

akien-mga commented Dec 15, 2020

[fbx] fix crash in FBX parser caused by mesh geometry #44371

[fbx] fix crash in FBX parser caused by mesh geometry #44371

Conversation

RevoluPowered commented Dec 14, 2020 • edited by akien-mga Loading

qarmin commented Dec 14, 2020

RevoluPowered commented Dec 14, 2020

qarmin commented Dec 14, 2020

RevoluPowered commented Dec 14, 2020 • edited Loading

qarmin commented Dec 14, 2020

RevoluPowered commented Dec 14, 2020 • edited Loading

RevoluPowered commented Dec 14, 2020

akien-mga commented Dec 15, 2020

RevoluPowered commented Dec 14, 2020 •

edited by akien-mga

Loading

RevoluPowered commented Dec 14, 2020 •

edited

Loading

RevoluPowered commented Dec 14, 2020 •

edited

Loading