Repository navigation
perf(String): Remove global locks on reference counters and replace them with atomic variables in string classes - #3495
Conversation
Mauller
added
Major
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughASCII and Unicode string buffer reference counts now use ChangesString reference-count synchronization
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Suggested reviewers: Merge Risk: 🟡 Moderate · up to The supported VC6 retail build uses ASCII and Unicode string buffers whose atomic reference counts are not constructed before use. Resolve this core string-lifetime risk before merging. Pre-merge checks ·
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly states the main change: replacing global string reference-counter locks with atomic variables. |
| Description check | |
The description directly explains the removal of critical-section locks and the use of atomic reference counters in both string classes. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
4ac97b0 to
3d8efcf
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| { | ||
| ScopedCriticalSection scopedCriticalSection(TheAsciiStringCriticalSection); | ||
|
|
||
| validate(); |
There was a problem hiding this comment.
🟠 High System/AsciiString.cpp:252
Concurrent assignment can increment the reference count of a buffer after another thread has freed it, causing a use-after-free and corrupting ownership. set reads stringSrc.m_data and increments its reference count without the critical-section guard; restore the guard around this ownership handoff.
| validate(); | |
| ScopedCriticalSection scopedCriticalSection(TheAsciiStringCriticalSection); | |
| validate(); |
Also found in 1 other location(s)
Core/GameEngine/Source/Common/System/UnicodeString.cpp:69
Removing the lock from
UnicodeStringcopy construction leaves copy-on-write mutation unsynchronized.ensureUniqueBufferOfSize()checksm_refCount == 1and then writes into the shared buffer; another thread can execute this constructor between that check and the write and increment the atomic count, so the mutating thread modifies storage now owned by both strings. Concurrent readers/writers can therefore observe corrupted string contents. Making only the counter atomic does not make the check-then-mutate sequence safe.
🤖 Copy this AI Prompt to have your agent fix this:
In file @Core/GameEngine/Source/Common/System/AsciiString.cpp around line 252:
Concurrent assignment can increment the reference count of a buffer after another thread has freed it, causing a use-after-free and corrupting ownership. `set` reads `stringSrc.m_data` and increments its reference count without the critical-section guard; restore the guard around this ownership handoff.
Also found in 1 other location(s):
- Core/GameEngine/Source/Common/System/UnicodeString.cpp:69 -- Removing the lock from `UnicodeString` copy construction leaves copy-on-write mutation unsynchronized. `ensureUniqueBufferOfSize()` checks `m_refCount == 1` and then writes into the shared buffer; another thread can execute this constructor between that check and the write and increment the atomic count, so the mutating thread modifies storage now owned by both strings. Concurrent readers/writers can therefore observe corrupted string contents. Making only the counter atomic does not make the check-then-mutate sequence safe.
There was a problem hiding this comment.
This remains unprotected: set increments stringSrc.m_data's refcount without synchronization, so concurrent destruction/assignment of stringSrc can still free it first.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Restore the string critical sections. · AsciiString.cpp:250-260
Core/GameEngine/Source/Common/System/AsciiString.cpp:250-260
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the string critical sections.
setreadsstringSrc.m_databefore incrementing its atomic reference count. If another thread callsclear()on the same source,releaseBuffer()can free the buffer between those operations. The atomic counter does not protect this pointer acquisition, so the increment can access freed memory.Restore the critical sections in both
setandreleaseBufferfor both string types.Suggested fix
+#include "Common/CriticalSection.h" #include "WWLib/utf8.h" @@ void AsciiString::releaseBuffer() { + ScopedCriticalSection scopedCriticalSection(TheAsciiStringCriticalSection); validate(); @@ void AsciiString::set(const AsciiString& stringSrc) { + ScopedCriticalSection scopedCriticalSection(TheAsciiStringCriticalSection); validate();+#include "Common/CriticalSection.h" #include "WWLib/utf8.h" @@ void UnicodeString::releaseBuffer() { + ScopedCriticalSection scopedCriticalSection(TheUnicodeStringCriticalSection); validate(); @@ void UnicodeString::set(const UnicodeString& stringSrc) { + ScopedCriticalSection scopedCriticalSection(TheUnicodeStringCriticalSection); validate();
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
be89b0e9-ebdb-4436-9965-319f2af367b8
📒 Files selected for processing (8)
Benchmarks/Google/Core/GoogleBenchmarkMain.cppCore/GameEngine/Include/Common/AsciiString.hCore/GameEngine/Include/Common/UnicodeString.hCore/GameEngine/Source/Common/System/AsciiString.cppCore/GameEngine/Source/Common/System/UnicodeString.cppGeneralsMD/Code/GameEngine/Include/Common/CriticalSection.hGeneralsMD/Code/GameEngine/Source/Common/System/CriticalSection.cppGeneralsMD/Code/Main/WinMain.cpp
💤 Files with no reviewable changes (5)
- GeneralsMD/Code/GameEngine/Include/Common/CriticalSection.h
- Core/GameEngine/Source/Common/System/UnicodeString.cpp
- Core/GameEngine/Source/Common/System/AsciiString.cpp
- GeneralsMD/Code/GameEngine/Source/Common/System/CriticalSection.cpp
- Benchmarks/Google/Core/GoogleBenchmarkMain.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
3d8efcf to
2201b8c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Acquire the source buffer before releasing the destination buffer. · AsciiString.cpp:250-260
Core/GameEngine/Source/Common/System/AsciiString.cpp:250-260
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAcquire the source buffer before releasing the destination buffer.
When two distinct
AsciiStringinstances share one buffer and callseton each other concurrently, both setters can decrement the shared reference count before either increments it. One setter can free the buffer, after which the other setter incrementsm_refCountthrough the freed pointer.UnicodeString::sethas the same ownership-transfer sequence and failure.Suggested fix
void AsciiString::set(const AsciiString& stringSrc) { validate(); if (&stringSrc != this) { + AsciiStringData* newData = stringSrc.m_data; + if (newData) + ++newData->m_refCount; releaseBuffer(); - m_data = stringSrc.m_data; - if (m_data) - ++m_data->m_refCount; + m_data = newData; } validate(); }void UnicodeString::set(const UnicodeString& stringSrc) { validate(); if (&stringSrc != this) { + UnicodeStringData* newData = stringSrc.m_data; + if (newData) + ++newData->m_refCount; releaseBuffer(); - m_data = stringSrc.m_data; - if (m_data) - ++m_data->m_refCount; + m_data = newData; } validate(); }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
8b549ab7-9cb7-4dee-b934-aea7464c230c
📒 Files selected for processing (2)
Core/GameEngine/Include/Common/AsciiString.hCore/GameEngine/Include/Common/UnicodeString.h
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
7ab9891 to
74a69ed
Compare
|
The bots still have a complaint about atomic initialization |
Just going through them, one looks like it was already an inherent problem, but i think it becomes more of an issue with the change to atomic variables. |
…with atomic variable (#3495)
…e with atomic variable (#3495)
74a69ed to
7ff8844
Compare
| if (--m_data->m_refCount == 0) | ||
| { | ||
| m_data->~AsciiStringData(); | ||
| TheDynamicMemoryAllocator->freeBytes(m_data); | ||
| } | ||
| m_data = nullptr; |
There was a problem hiding this comment.
Is this safe? Imagine the following scenario:
Thread_1:
//MAYBE_UNUSED AsciiString str1("test");
if (str1.m_data)
{
if (--str1.m_data->m_refCount == 0)
{
str1.m_data->~AsciiStringData();
TheDynamicMemoryAllocator->freeBytes(str1.m_data);
// <-- thread_1 is here
}
str1.m_data = nullptr;
}Thread_2:
// thread_2 holds a reference to `str` and executes this before thread_1 can reset the data pointer
MAYBE_UNUSED AsciiString str2 = std::ref(str1);Thread_2 would access the data m_data points to, even though it's already freed, so use-after-free.
There was a problem hiding this comment.
Yes this looks like a race. Needs to null str1.m_data first and cache it in a local var before touching the reference counter.
3 participants
Merge with rebase
This PR removes the critical section global locks from the AsciiString and UnicodeString classes.
In their place we use atomic counters for managing the reference counters.