godotengine / godotengine/godot

[Core] `Array` and `Dictionary` read only behavior is unsafe

Open
#93,702 10 comments 2 reactions 0 assignees View on GitHub
bug discussion topic:core
Dominant language
C++
Stars
117k
Forks
26.8k
PR merge metrics
PR metrics pending

Description

### Tested versions

N/A

### System information

N/A

### Issue description

For example:
* https://github.com/godotengine/godot/pull/93636

You also have that, in C++, the following is broken, for example:
```cpp
Array tmp;
tmp.push_back(1);
tmp.push_back(2);
tmp.make_read_only();
assert(tmp[0] != tmp[1]);
```

All due to the fact that the return from the subscript operator and `Array::get` return a reference to a single internal `read_only` value

This would also cause problems in cases like:
```cpp
const Variant &first_value = arr[0];

for (int i = 1; i < arr.size(); ++i) {
// Do something involving access to `arr`, if the array is read-only it will change the value in `first_value`.
}
```

The main problem is that the non-const subscript operator has to return a non-const reference, if this wasn't necessary we could safely return `const Variant &` for the const operator and just trust that no one messes with it with casting, just like the const operator for `LocalVector`, but since we might be working with a non-const but read-only `Array` reference we can't reasonably error or crash when accessing the non-const operator.

---

A possible temporary improvement would be to remove the use of the read-only data in the const access cases (this is already the case in `Dictionary`), that would reduce the impact somewhat, but the non-const case is more tricky.

### Steps to reproduce

N/A

### Minimal reproduction project (MRP)

N/A

Contributor guide

Open the contributing guide

Research direction

Start by tracing the C++ implementations of Array and Dictionary subscript access and Array::get, then compare their const-access behavior with LocalVector. Reproduce the read-only Array examples from the issue and establish a design in which accesses do not share a mutable internal value or unexpectedly change an existing reference.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.