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 ofkCFStringEncodingInvalidId
instead 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_once
or 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 otherCFString
initialization 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).