godotengine / godotengine/godot
Potential data race in `ObjectDB::add_instance` when unlocking before re-evaluating error macro condition
- Dominant language
- C++
- Stars
- 117k
- Forks
- 26.8k
- PR merge metrics
- PR metrics pending
Description
### Tested versions
- Branch 4.5, commit [c834443ef1fa3516e30124d8afaf448353d31010](https://github.com/godotengine/godot/commit/c834443ef1fa3516e30124d8afaf448353d31010)
### System information
Windows 10, 4.5.1.rc.custom_build.c834443ef
### Issue description
Hello Godot community,
I'm new here. I'm interested in game engine develop, so I am currently learning the source code of Godot, and I think I found a subtle multi-threading bug in `ObjectDB::add_instance`.
Relevant code:
https://github.com/godotengine/godot/blob/c834443ef1fa3516e30124d8afaf448353d31010/core/object/object.cpp#L2445-L2449
Problem:
- The condition `object_slots[slot].object != nullptr` is evaluated once while the spin lock is held, then the lock is released.
- The macro `ERR_FAIL_COND_V` evaluates the condition again after the unlock.
- Another thread could call `remove_instance` or other functions in the small window between the manual unlock and the macro evaluation, setting `object_slots[slot].object` back to `nullptr`.
- If that happens, the macro no longer triggers, and execution continues without the lock held, leading to unsynchronized writes to `object_slots[slot]` (data race and potential memory corruption).
Proposed fix: use `ERR_FAIL_V_MSG` instead:
```diff
uint32_t slot = object_slots[slot_count].next_free;
if (object_slots[slot].object != nullptr) {
spin_lock.unlock();
- ERR_FAIL_COND_V(object_slots[slot].object != nullptr, ObjectID());
+ ERR_FAIL_V_MSG(ObjectID(), "Engine ran out of available object slots.");
}
```
Let me know if this analysis makes sense. I’d be happy to open a PR with the change.
### Steps to reproduce
Found this one via reading the source code, and due to race condition, hard to give a reproduce example.
### Minimal reproduction project (MRP)
Found this one via reading the source code, and due to race condition, hard to give a reproduce example.
Contributor guide
Research direction
Start in core/object/object.cpp at ObjectDB::add_instance around the linked lines, then read the nearby locking and error-handling code alongside remove_instance. Verify the condition is not re-evaluated after the lock is released, while preserving the out-of-object-slots failure behavior; completion should include a review of the concurrent access path since no reproduction is provided.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- game-dev
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100