Fix Protected Value Property Access Safety #3505
Merged
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.
Goals ⚽
This PR refactors the codebase to always access
Protected
properties through the wrapper when accessing sub properties. That isprotectedValue.property
must always be accessed as$protectedValue.property
to the full access is protected by the lock.Essentially,
lock.around { value }
is roughly equivalent towhich necessarily puts any access to the returned
value
's properties outside the lock. Instead, using theKeyPath
-baseddynamicMemberLookup
ensures the entire access is protected, as it's roughly equivalent to:Implementation Details 🚧
In addition to auditing the use sites, various
Protected
APIs were updated to supportthrows
/rethrows
, to enable them to wrap throwing APIs. Namely,MultipartUpload
has some troublesome internal API that access aProtected
value while building the multipart payload, meaning the entire access must be protected. Both of these (let data = try $multipartFormData.read { try $0.encode() }
andtry $multipartFormData.read { try $0.writeEncodedData(to: fileURL) }
) perform unbounded work inside the lock, which could have troublesome performance implications. However, without extensive refactoring of the multipart payload APIs, changes to the functionality isn't possible. This work was already being performed in the background, so overall impact should be minimal.Testing Details 🔍
Additional tests were added, including one larger, high contention test which triggered the thread sanitizer when using a wrapped value instead of the wrapper.