Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a possible crash when unreferencing an empty CowData<T> #87233

Closed
wants to merge 1 commit into from

Conversation

bs-mwoerner
Copy link
Contributor

Fixes #87076.

CowData<T>::_unref() was 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);
}

_FORCE_INLINE_ SafeNumeric<uint32_t> *_get_refcount() const {
	if (!_ptr) {
		return nullptr;
	}

	return reinterpret_cast<SafeNumeric<uint32_t> *>(_ptr) - 2;
}

On the surface, if (!p_data) doesn't do anything because Memory::free_static() uses its own null check and the parameter is otherwise unused, so in the release builds of Godot 4.2.1 and 4.3 dev 1 (and most likely others), there is at least one occasion where this check is optimized out by the compiler. However, the method assumes that p_data == this->_ptr, so that if (!p_data) is an implicit check for if (this->_ptr != nullptr). If the compiler, unaware of this assumption, removes the if (!p_data), then if (refc->decrement() > 0) dereferences a null pointer whenever this->_ptr is null.

The p_data parameter makes it possible to decrement the reference count of the internal pointer but then free a different pointer should that reference count reach zero. I'm not sure what the original use case for this was, especially since the method may crash should you pass in anything other than this->ptr. In the current code, the method is only ever called as _unref(_ptr).

I removed the p_data parameter and made the method explicitly operate on this->_ptr. This should prevent the compiler from optimizing the check out although I don't know how to create an official release build, so I can't check the generated code.

Since CowData<T> is the internal data structure used in String and Vector<T>, this change affects basically the whole engine. Hopefully in a good way.

… if an indirect null pointer check is optimized out by the compiler.
@bs-mwoerner bs-mwoerner requested a review from a team as a code owner January 15, 2024 20:17
@AThousandShips AThousandShips added this to the 4.3 milestone Jan 15, 2024
@bs-mwoerner bs-mwoerner marked this pull request as draft January 15, 2024 23:54
@bs-mwoerner
Copy link
Contributor Author

Setting this to draft for now because of a report that the crash is still there in a release build that includes this change and because I don't quite agree with the logic of my explanation for why the compiler might remove the null check. I'd welcome any comments and insights.

This is the relevant code that triggers the crash in the linked issue:

RBMap<uint64_t, Vector<Vector<uint8_t>>> sorted_packets;
while (true)
{
    // [...]
    PackedByteArray data;
    data.resize(packet.bytes);
    memcpy(data.ptrw(), packet.packet, packet.bytes);
    sorted_packets[granule_pos].push_back(data);
    packet_count++;
}

From what I gather from the assembly, this is what the compiler does when the scope ends (pseudocode):

packet_count = packet_count + 1;
if (tempdata._cowdata._ptr != nullptr)
{
    decrement the reference count at tempdata._cowdata._ptr - 8
    if the reference count is now 0, free tempdata._cowdata._ptr

    if (data._cowdata._ptr != nullptr)
        refcptr = data._cowdata._ptr - 8
    else
        refcptr = nullptr
}
else
{
    // This branch is taken just before the crash
    if (r12 != nullptr)
        refcptr = r12 - 8
    else
        refcptr = nullptr
}
decrement the reference count at refcptr // This crashes on the final loop because r12 and thus refcptr is null
if the reference count is now 0, free r12
goto top_of_while

r12 is the first parameter that was passed to memcpy(data.ptrw(), packet.packet, packet.bytes), which is the same value as data._cowdata._ptr and tempdata._cowdata._ptr (tempdata is probably some temporary copy created when copy-constructing the Vector in the map). The compiler seems to be certain that a null check is not necessary when doing data._cowdata._unref(data._cowdata._ptr), but I can't see why.

@bs-mwoerner
Copy link
Contributor Author

So, after noticing that my own production builds with Visual Studio don't crash, realizing that Godot's Windows binaries are built with gcc, and reading up on gcc quirks, I learned that gcc indeed considers a pointer to be proven non-null if it has ever been passed to memcpy, even if that memcpy was a zero-byte nop and didn't touch or check the pointer. So there's actually nothing wrong with CowData<T>. The cause of the linked crash is likely that when compiling with gcc, the zero-byte memcpy silently removes the null check when data is eventually cleaned up after going out of scope. Therefore, ResourceImporterOggVorbis must make sure that the data is not zero-length before attempting to "copy" it into the array.

Sorry about the turmoil. I didn't expect the compiler to be so bold in its optimizations and didn't even consider that as a possible side effect. I'll open a new pull request for the fix in ResourceImporterOggVorbis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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