godotengine / godotengine/godot

Potential data race in `ObjectDB::add_instance` when unlocking before re-evaluating error macro condition

Open
#111,173 0 comments 0 reactions 0 assignees View on GitHub
bug topic:core
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.