godotengine / godotengine/godot-cpp
[Godot 3.x] Copying CharString wrappers causes premature deallocation of its data
- 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.