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 race condition in __CFStringGetEightBitStringEncoding #5155

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

jmschonfeld
Copy link
Contributor

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 of kCFStringEncodingInvalidId 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:

  • In the swift-testing test runner produced by SwiftPM there are no calls to __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 argument
  • When calling CFStringAppendCString (what String(format:) uses to build the output buffer) with kCFStringEncodingInvalidId, the function silently fails and does nothing

This 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 use dispatch_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 other CFString 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).

@jmschonfeld
Copy link
Contributor Author

@swift-ci please smoke test

@jmschonfeld
Copy link
Contributor Author

@swift-ci please smoke test Linux platform

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test Linux platform

@jmschonfeld
Copy link
Contributor Author

@swift-ci please smoke test Linux platform

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test Linux platform

1 similar comment
@jmschonfeld
Copy link
Contributor Author

@swift-ci please test Linux platform

@jmschonfeld jmschonfeld merged commit c466923 into swiftlang:main Jan 15, 2025
2 checks passed
@jmschonfeld jmschonfeld deleted the cfstring-8bit-race branch January 15, 2025 17:20
jmschonfeld added a commit to jmschonfeld/swift-corelibs-foundation that referenced this pull request Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants