Skip to content

Commit ad4e777

Browse files
committed
Eliminate race in swift_bridgeErrorToNSError.
Huge thanks to John for noting that 'consume' didn't provide the guarantees we wanted, and to Michael G. for getting a TSan build up and running to identify/verify this race. It's possible that this is overlay strict, and that we only need to look at the domain to ensure that the code and userInfo are visible. However, TSan seems to prefix the form in this patch, so we'll be more conservative for now. Fixes rdar://problem/27541751. (cherry picked from commit e07e887)
1 parent 682b5e2 commit ad4e777

File tree

2 files changed

+13
-12
lines changed

2 files changed

+13
-12
lines changed

stdlib/public/runtime/ErrorObject.mm

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -79,15 +79,10 @@ - (NSInteger)code {
7979
- (NSDictionary*)userInfo {
8080
auto error = (const SwiftError*)self;
8181
auto userInfo = error->userInfo.load(SWIFT_MEMORY_ORDER_CONSUME);
82-
83-
if (userInfo) {
84-
// Don't need to .retain.autorelease since it's immutable.
85-
return (NSDictionary*)userInfo;
86-
} else {
87-
// -[NSError userInfo] never returns nil on OS X 10.8 or later.
88-
NSDictionary *emptyDict = SWIFT_LAZY_CONSTANT(@{});
89-
return emptyDict;
90-
}
82+
assert(userInfo
83+
&& "Error box used as NSError before initialization");
84+
// Don't need to .retain.autorelease since it's immutable.
85+
return (NSDictionary*)userInfo;
9186
}
9287

9388
- (id)copyWithZone:(NSZone *)zone {
@@ -319,8 +314,11 @@ typedef SWIFT_CC(swift)
319314
static id _swift_bridgeErrorToNSError_(SwiftError *errorObject) {
320315
auto ns = reinterpret_cast<NSError *>(errorObject);
321316

322-
// If we already have a domain set, then we've already initialized.
323-
if (errorObject->domain.load(SWIFT_MEMORY_ORDER_CONSUME))
317+
// If we already have a domain and userInfo set, then we've already
318+
// initialized.
319+
// FIXME: This might be overly strict; can we look only at the domain?
320+
if (errorObject->domain.load(std::memory_order_acquire) &&
321+
errorObject->userInfo.load(std::memory_order_acquire))
324322
return ns;
325323

326324
// Otherwise, calculate the domain and code (TODO: and user info), and
@@ -334,6 +332,10 @@ static id _swift_bridgeErrorToNSError_(SwiftError *errorObject) {
334332
NSDictionary *userInfo =
335333
swift_stdlib_getErrorUserInfoNSDictionary(value, type, witness);
336334

335+
// Never produce an empty userInfo dictionary.
336+
if (!userInfo)
337+
userInfo = SWIFT_LAZY_CONSTANT(@{});
338+
337339
// The error code shouldn't change, so we can store it blindly, even if
338340
// somebody beat us to it. The store can be relaxed, since we'll do a
339341
// store(release) of the domain last thing to publish the initialized

validation-test/stdlib/ErrorProtocol.swift

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
// RUN: %target-run-simple-swift
22
// REQUIRES: executable_test
3-
// REQUIRES: rdar27541751
43
// REQUIRES: objc_interop
54

65
import SwiftPrivate

0 commit comments

Comments
 (0)