Godot crashes when importing OGG file or renaming folder / OGG Vorbis (release builds only)

[DevMimas]

Tested versions

Godot Engine v4.2.1.stable.mono.official

System information

Windows 10 - Godot v4.2.1.stable.mono.official (Forward+)

Issue description

Godot Editor crashes (v4.2.1),

• if you try to import an OGG file
• if you rename the folder in which the OGG file is located. Reimport causes crash

If you remove the OGG file from the project folder, the project is loaded successfully.

Better description needed
I would also like the error text to be better defined. For Lain it is absolutely meaningless.
See also my proposal: godotengine/godot-proposals#8808
My commit in the pull request: #84723

Vide open OGG file project

2024-01-15.23-26-39.mp4

Steps to reproduce

N/A

Minimal reproduction project (MRP)

OGG TestProject.zip

[alessandrofama]

It would have been nice if this issue included an example project or an .ogg file to verify the crash.

I can reproduce the crash with this project ogg_error_spam.zip from #84712 on Godot v4.2.1.stable.mono.official and Godot Engine v4.2.1.stable.official just by importing the file the first time, by double clicking the audio file & renaming the folder in which the file is located, which i guess triggers a re-import?

I can't reproduce the crash on latest 3524346 (v4.3.dev)
Godot will print Error during vorbis synthesis -135 but will not crash.

Important to note that the crash seems to only be triggered by corrupt .ogg files and not any .ogg file as the description implies.

(Windows 10.0.22621)

[bs-mwoerner]

I get an Access Violation in both 4.2.1 and 4.3 Dev 1 when importing the file from that ZIP archive, but only in the release builds, not in my debug build. That may be a bit tricky to debug.

Error -135 by the way means that the file contains non-audio data (comments?). I suppose that's an internal detail and not necessarily something that needs to be reported to the user.

[RichTeaMan]

I've also seen this problem manifest when cloning my exsting Godot project. If the .godot/import directory doesn't already have ogg import files the entire Godot editor segfaults. I have observed this on several Ubuntu computers and Github CI.

[bs-mwoerner]

I did some assembly-level debugging on the release exe. So far it looks like the crash happens shortly after processing a zero-length packet in the OGG file, even though zero-length packets cause no issues in the debug builds. The actual cause for the crash seems to be trying to decrement the reference count of a zero pointer in a PackedByteArray when it goes out of scope, which should not be possible without an inter-thread race condition. Not sure how that happens, yet. Might be some kind of memory corruption.

[bs-mwoerner]

Uh, this is kind of wild. First of all: I think the crash could be prevented just by adding an if (packet.bytes > 0), but I think the actual problem runs much deeper and this may not be the best place to fix this.

A packet.bytes of 0 gives an empty PackedByteArray with an internal _cowdata._ptr == nullptr, which is well defined and should not cause any problems, so the actual question is: If all methods involved correctly check for nullptr and a debug build has no problem with the data pointer being null, why do the release builds crash? After staring at the code for hours, here's my theory.

The crash happens in CowData<T>::_unref(). This is implemented like this:

template <class T>
void CowData<T>::_unref(void *p_data) {
	if (!p_data) {
		return;
	}

	SafeNumeric<uint32_t> *refc = _get_refcount();

	if (refc->decrement() > 0) {
		return; // still in use
	}
	// clean up

	if (!std::is_trivially_destructible<T>::value) {
		uint32_t *count = _get_size();
		T *data = (T *)(count + 1);

		for (uint32_t i = 0; i < *count; ++i) {
			// call destructors
			data[i].~T();
		}
	}

	// free mem
	Memory::free_static((uint8_t *)p_data, true);
}

At first glance, it appears a bit suspicious that a null check is done only for the method parameter p_data but not for the pointer returned from _get_refcount(), which is derived from this->_ptr. I think the rationale here is that:

• _unref() is a private method.
• Every time it is called, this->_ptr is passed in as p_data, so the method works under the implicit assumption that p_data == this->_ptr and thus a check for p_data == nullptr is equivalent to a check for this->_ptr == nullptr.

Spoiler alert: The C++ compiler does not make that assumption.

The crash occurs in an inlined version of the method and the actual code being executed looks more like this:

SafeNumeric<uint32_t> *refc = this->_ptr ?
	reinterpret_cast<SafeNumeric<uint32_t> *>(_ptr) - 2 :
	nullptr;

if (refc->decrement() > 0) {
	return; // still in use
}

Memory::free_static((uint8_t *)p_data, true);

Note that the check for if (!p_data) is optimized out. If the method is called while this->_ptr == nullptr, there is no effective check to prevent a null pointer dereference in refc->decrement(). That's exactly what happens with those specific OGG files.

So is this a bug in the compiler? Isn't this code semantically different from the C++ source, especially, since p_data is used later in the method? That's what I thought initially, but I think the key here is that Memory::free_static() also checks its parameter for null, so the additional check in the caller is indeed redundant. The compiler does not see that checking the function parameter is supposed to be a check of a member variable, because all calls to the function pass the value of that member.

This might be kind of a big deal, since CowData is used all over Godot. Or it might not, because the bug requires the code to be optimized in a certain way to actually trigger. Nevertheless, this issue proves that there are situations where it does happen.

[DevMimas]

Hello everyone,
Thank you for your feedback.

Sorry, I was busy, so it took a while.
I have now added a test project with my OGG file + a video. The project crashes immediately. Even when I created the test project and dragged the file into the folder, the project crashed immediately.

What is a corrupt .ogg file? A file that is damaged?

I can open and play the file on Windows 10, just as a hint.

[RichTeaMan]

@bs-mwoerner thanks for your deep dive into the code, it was an interesting read.

I've built your code in the linked pull request (a production build using mono, scons platform=linuxbsd target=editor production=yes module_mono_enabled=yes), but it appears I'm still getting the same error:

ERROR: Caller thread can't call this function in this node (/root). Use call_deferred() or call_thread_group() instead.
   at: propagate_notification (scene/main/node.cpp:2281)

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.3.dev.mono.custom_build (f6ae492523699cabbe59175f72839b30d0be573e)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /usr/lib/dotnet/shared/Microsoft.NETCore.App/7.0.15/libcoreclr.so(+0x621a5e) [0x7f5e73621a5e] (??:0)
[2] /usr/lib/dotnet/shared/Microsoft.NETCore.App/7.0.15/libcoreclr.so(+0x6210d5) [0x7f5e736210d5] (??:0)
[3] /lib/x86_64-linux-gnu/libc.so.6(+0x42910) [0x7f5ec6c42910] (??:0)
[4] /home/tom/projects/backup/godot/bin/godot.linuxbsd.editor.x86_64.mono(+0x673b1c) [0x55decc69db1c] (??:0)
[5] /home/tom/projects/backup/godot/bin/godot.linuxbsd.editor.x86_64.mono(+0x674876) [0x55decc69e876] (??:0)
[6] /home/tom/projects/backup/godot/bin/godot.linuxbsd.editor.x86_64.mono(+0x6750b0) [0x55decc69f0b0] (??:0)
[7] /home/tom/projects/backup/godot/bin/godot.linuxbsd.editor.x86_64.mono(+0x1368a5b) [0x55decd392a5b] (??:0)
[8] /home/tom/projects/backup/godot/bin/godot.linuxbsd.editor.x86_64.mono(+0x136b1f6) [0x55decd3951f6] (??:0)
[9] /home/tom/projects/backup/godot/bin/godot.linuxbsd.editor.x86_64.mono(+0x400bfb6) [0x55ded0035fb6] (??:0)
[10] /home/tom/projects/backup/godot/bin/godot.linuxbsd.editor.x86_64.mono(+0x400c76c) [0x55ded003676c] (??:0)
[11] /home/tom/projects/backup/godot/bin/godot.linuxbsd.editor.x86_64.mono(+0x3b3101d) [0x55decfb5b01d] (??:0)
[12] /home/tom/projects/backup/godot/bin/godot.linuxbsd.editor.x86_64.mono(+0x4718803) [0x55ded0742803] (??:0)
[13] /lib/x86_64-linux-gnu/libc.so.6(+0x97ada) [0x7f5ec6c97ada] (??:0)
[14] /lib/x86_64-linux-gnu/libc.so.6(+0x12847c) [0x7f5ec6d2847c] (??:0)
-- END OF BACKTRACE --
================================================================