Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix data race in PagedArray #86412

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Conversation

stuartcarnie
Copy link
Contributor

@stuartcarnie stuartcarnie commented Dec 22, 2023

This PR resolves a data race in the PagedArrayPool and PagedArray types. The thread sanitiser identified the data race via the page_pool field of the PagedArrayPool class. The write occurred:

page_pool = (T **)memrealloc(page_pool, sizeof(T *) * pages_allocated);

and the concurrent read here:

return page_pool[p_page_id];

The actual data race of the page_pool pointer being access concurrently is possibly safe, as it is a word read / write, however, an additional problem is the underlying memory in the memrealloc from the old allocation is being immediately zero'd in debug builds on macOS, so the call the get_page is sometimes returning an nullptr. In a release build, the old allocation may be immediately reused before the call to get_page, returning an invalid pointer, resulting in crashes, memory corruption, etc.

@stuartcarnie stuartcarnie requested a review from a team as a code owner December 22, 2023 00:05
@fire fire changed the title fix: data race in PagedArray Fix data race in PagedArray Dec 22, 2023
@AThousandShips AThousandShips added this to the 4.3 milestone Dec 22, 2023
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good besides the little nitpick.

@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 2, 2024
@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

@stuartcarnie
Copy link
Contributor Author

@RandomShaper squashed

@akien-mga akien-mga merged commit a97b01c into godotengine:master Jan 3, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉 -- hopefully the start of many, given what I know you've been cooking ;)

@stuartcarnie stuartcarnie deleted the sgc/data_race branch January 3, 2024 09:34
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants