godotengine / godotengine/godot-cpp

[Godot 3.x] Copying CharString wrappers causes premature deallocation of its data

Open
#973 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
2.7k
Forks
809
Avg merge
1d 3h
Merged PRs (30d)
8

Description

I figured this out while modifying the GDNative version of GodotSteam.
Here is an interesting snippet of code:

```
//! Sets a key/value pair in the lobby metadata.
bool Steam::setLobbyData(uint64_t steam_lobby_id, String key, String value){
if(SteamMatchmaking() == NULL){
return false;
}
CSteamID lobby_id = (uint64)steam_lobby_id;
auto result = SteamMatchmaking()->SetLobbyData(lobby_id, key.utf8().get_data(), value().utf8().get_data());
return result;
}
```

Assuming the key and values are "my_key" and "my_value", when compiling with MSVC, I get "my_key" and "my_key" as the key and value, respectively.
The issue is that MSVC is not doing copy-elision at this line here: https://github.com/godotengine/godot-cpp/blob/3.x/src/core/String.cpp#L203.

Hence the destructor is called, and the ref-count goes to zero just after the first `utf8()` call. The second one will thus get the same memory for its COW object, and it will overwrite it.

This, of course, happens every time you copy the object, but given the missing copy-elision, this makes those CharString unusable in MSVC, not even as temporary.

Ideally, a copy constructor in CharString should call an API method to increment the ref-count. A quick workaround is to ensure copy-elision is enforced in the utf8() method call by MSVC by initializing the CharString with a suitable constructor in the same return statement (the workaround I'm using right now).
This will ensure it can be used as a temporary object (and maybe make it non- copyable?).

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.