Fix race condition in __CFStringGetEightBitStringEncoding #5155
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a longstanding bug in which when multiple threads race calling
__CFStringGetEightBitStringEncoding()for the first time. When some threads that lose the race (but make the call before the winning thread finishes executing its call) will the losing threads receive a return value ofkCFStringEncodingInvalidIdinstead of a valid string encoding. This is because__CFStringGetEightBitStringEncoding()ignores the return value of__CFStringComputeEightBitStringEncoding()and instead always returns the value in the global__CFDefaultEightBitStringEncoding. However, when a thread loses the race,__CFStringComputeEightBitStringEncoding()returns a temporary value (ASCII) without setting the global (since another thread is working on calculating the global). This fixes the issue by always respecting the return value of__CFStringComputeEightBitStringEncoding().This resulted in the symptoms described in #5152 because when executing multiple swift-testing tests in parallel that each call
String(format:), the call can nondeterministically return an empty/incorrect string because:__CFStringGetEightBitStringEncoding()before the unit test starts executing (in other words, any calls within the unit tests themselves can trigger this race)String(format:)calls__CFStringGetEightBitStringEncoding()to determine the encoding of the temporary buffer created for the printed value of each numeric argumentCFStringAppendCString(whatString(format:)uses to build the output buffer) withkCFStringEncodingInvalidId, the function silently fails and does nothingThis leads to
String(format:)creating a race between each test execution on the call to__CFStringGetEightBitStringEncoding()and the losing threads fail to append contents to the output string. This doesn't reproduce with XCTest or with the Xcode test runner because they each (indirectly) cause calls to__CFStringGetEightBitStringEncoding()before the test contents execute, thus "warming" the value of the global and preventing the race from occurring in test code.As a side note,
__CFStringComputeEightBitStringEncoding()appears quite thread-unsafe itself as it does not usedispatch_onceor any robust locking primitives to guard against races. However I don't want to try to rewrite that at the moment as it is intertwined with a few otherCFStringinitialization functions that seem to have very precarious threading considerations that I fear are likely to break clients if I change them too much without some more investigation and careful consideration (but we should do this eventually).