Merge pull request #63646 from lorentey/fix-string-bitcast

[stdlib] Fix potentially undefined behavior in StringObject.nativeStorage and document Builtin.unsafeBitCast

Author: lorentey

stdlib/public/core/Builtin.swift

@@ -72,14 +72,23 @@ func _canBeClass<T>(_: T.Type) -> Int8 {
 ///   `unsafeBitCast(_:to:)` with class or pointer types; doing so may
 ///   introduce undefined behavior.
 ///
-/// - Warning: Calling this function breaks the guarantees of the Swift type
-///   system; use with extreme care.
+/// Warning: Calling this function breaks the guarantees of the Swift type
+/// system; use with extreme care.
 ///
-/// - Parameters:
+/// Warning: Casting from an integer or a pointer type to a reference type
+/// is undefined behavior. It may result in incorrect code in any future
+/// compiler release. To convert a bit pattern to a reference type:
+/// 1. convert the bit pattern to an UnsafeRawPointer.
+/// 2. create an unmanaged reference using Unmanaged.fromOpaque()
+/// 3. obtain a managed reference using Unmanaged.takeUnretainedValue()
+/// The programmer must ensure that the resulting reference has already been
+/// manually retained.
+///
+/// Parameters:
 ///   - x: The instance to cast to `type`.
 ///   - type: The type to cast `x` to. `type` and the type of `x` must have the
 ///     same size of memory representation and compatible memory layout.
-/// - Returns: A new instance of type `U`, cast from `x`.
+/// Returns: A new instance of type `U`, cast from `x`.
 @inlinable // unsafe-performance
 @_transparent
 public func unsafeBitCast<T, U>(_ x: T, to type: U.Type) -> U {

stdlib/public/core/StringGuts.swift

@@ -123,7 +123,7 @@ extension _StringGuts {
   // Whether this string has breadcrumbs
   internal var hasBreadcrumbs: Bool {
     return hasSharedStorage
-      || (hasNativeStorage && _object.nativeStorage.hasBreadcrumbs)
+      || (hasNativeStorage && _object.withNativeStorage { $0.hasBreadcrumbs })
   }
 }
 
@@ -249,12 +249,11 @@ extension _StringGuts {
   ) -> Int? {
     #if _runtime(_ObjC)
     // Currently, foreign  means NSString
-    if let res = _cocoaStringCopyUTF8(_object.cocoaObject,
-      into: UnsafeMutableRawBufferPointer(start: mbp.baseAddress,
-                                          count: mbp.count)) {
-      return res
+    let res = _object.withCocoaObject {
+      _cocoaStringCopyUTF8($0, into: UnsafeMutableRawBufferPointer(mbp))
     }
-    
+    if let res { return res }
+
     // If the NSString contains invalid UTF8 (e.g. unpaired surrogates), we
     // can get nil from cocoaStringCopyUTF8 in situations where a character by
     // character loop would get something more useful like repaired contents

stdlib/public/core/StringGutsRangeReplaceable.swift

@@ -13,30 +13,42 @@
 // COW helpers
 extension _StringGuts {
   internal var nativeCapacity: Int? {
+    @inline(never)
+    @_effects(releasenone)
+    get {
       guard hasNativeStorage else { return nil }
-      return _object.nativeStorage.capacity
+      return _object.withNativeStorage { $0.capacity }
+    }
   }
 
   internal var nativeUnusedCapacity: Int? {
+    @inline(never)
+    @_effects(releasenone)
+    get {
       guard hasNativeStorage else { return nil }
-      return _object.nativeStorage.unusedCapacity
+      return _object.withNativeStorage { $0.unusedCapacity }
+    }
   }
 
   // If natively stored and uniquely referenced, return the storage's total
   // capacity. Otherwise, nil.
   internal var uniqueNativeCapacity: Int? {
-    @inline(__always) mutating get {
+    @inline(never)
+    @_effects(releasenone)
+    mutating get {
       guard isUniqueNative else { return nil }
-      return _object.nativeStorage.capacity
+      return _object.withNativeStorage { $0.capacity }
     }
   }
 
   // If natively stored and uniquely referenced, return the storage's spare
   // capacity. Otherwise, nil.
   internal var uniqueNativeUnusedCapacity: Int? {
-    @inline(__always) mutating get {
+    @inline(never)
+    @_effects(releasenone)
+    mutating get {
       guard isUniqueNative else { return nil }
-      return _object.nativeStorage.unusedCapacity
+      return _object.withNativeStorage { $0.unusedCapacity }
     }
   }
 
@@ -54,6 +66,20 @@ extension _StringGuts {
 
 // Range-replaceable operation support
 extension _StringGuts {
+  @inline(__always)
+  internal mutating func updateNativeStorage<R>(
+    _ body: (__StringStorage) -> R
+  ) -> R {
+    let (r, cf) = self._object.withNativeStorage {
+      let r = body($0)
+      let cf = $0._countAndFlags
+      return (r, cf)
+    }
+    // We need to pick up new count/flags from the modified storage.
+    self._object._setCountAndFlags(to: cf)
+    return r
+  }
+
   @inlinable
   internal init(_initialCapacity capacity: Int) {
     self.init()
@@ -243,11 +269,7 @@ extension _StringGuts {
   internal mutating func appendInPlace(
     _ other: UnsafeBufferPointer<UInt8>, isASCII: Bool
   ) {
-    self._object.nativeStorage.appendInPlace(other, isASCII: isASCII)
-
-    // We re-initialize from the modified storage to pick up new count, flags,
-    // etc.
-    self = _StringGuts(self._object.nativeStorage)
+    updateNativeStorage { $0.appendInPlace(other, isASCII: isASCII) }
   }
 
   @inline(never) // slow-path
@@ -256,11 +278,7 @@ extension _StringGuts {
     _internalInvariant(self.uniqueNativeUnusedCapacity != nil)
 
     var iter = Substring(other).utf8.makeIterator()
-    self._object.nativeStorage.appendInPlace(&iter, isASCII: other.isASCII)
-
-    // We re-initialize from the modified storage to pick up new count, flags,
-    // etc.
-    self = _StringGuts(self._object.nativeStorage)
+    updateNativeStorage { $0.appendInPlace(&iter, isASCII: other.isASCII) }
   }
 
   internal mutating func clear() {
@@ -270,8 +288,7 @@ extension _StringGuts {
     }
 
     // Reset the count
-    _object.nativeStorage.clear()
-    self = _StringGuts(_object.nativeStorage)
+    updateNativeStorage { $0.clear() }
   }
 
   internal mutating func remove(from lower: Index, to upper: Index) {
@@ -281,10 +298,7 @@ extension _StringGuts {
     _internalInvariant(lowerOffset <= upperOffset && upperOffset <= self.count)
 
     if isUniqueNative {
-      _object.nativeStorage.remove(from: lowerOffset, to: upperOffset)
-      // We re-initialize from the modified storage to pick up new count, flags,
-      // etc.
-      self = _StringGuts(self._object.nativeStorage)
+      updateNativeStorage { $0.remove(from: lowerOffset, to: upperOffset) }
       return
     }
 
@@ -412,8 +426,9 @@ extension _StringGuts {
 
     let start = bounds.lowerBound._encodedOffset
     let end = bounds.upperBound._encodedOffset
-    _object.nativeStorage.replace(from: start, to: end, with: codeUnits)
-    self = _StringGuts(_object.nativeStorage)
+    updateNativeStorage {
+      $0.replace(from: start, to: end, with: codeUnits)
+    }
     return Range(_uncheckedBounds: (start, start + codeUnits.count))
   }
 
@@ -435,9 +450,10 @@ extension _StringGuts {
 
     let start = bounds.lowerBound._encodedOffset
     let end = bounds.upperBound._encodedOffset
-    _object.nativeStorage.replace(
-      from: start, to: end, with: codeUnits, replacementCount: replCount)
-    self = _StringGuts(_object.nativeStorage)
+    updateNativeStorage {
+      $0.replace(
+        from: start, to: end, with: codeUnits, replacementCount: replCount)
+    }
     return Range(_uncheckedBounds: (start, start + replCount))
   }
 

stdlib/public/core/StringObject.swift

@@ -172,6 +172,17 @@ internal struct _StringObject {
     _internalInvariant(!isSmall)
     return CountAndFlags(rawUnchecked: _countAndFlagsBits)
   }
+
+  // FIXME: This ought to be the setter for property `_countAndFlags`.
+  @_alwaysEmitIntoClient
+  internal mutating func _setCountAndFlags(to value: CountAndFlags) {
+#if arch(i386) || arch(arm) || arch(arm64_32) || arch(wasm32)
+    self._count = value.count
+    self._flags = value.flags
+#else
+    self._countAndFlagsBits = value._storage
+#endif
+  }
 }
 
 // Raw
@@ -894,6 +905,13 @@ extension _StringObject {
       discriminatedObjectRawBits & Nibbles.largeAddressMask)
   }
 
+  @inline(__always)
+  @_alwaysEmitIntoClient
+  internal var largeAddress: UnsafeRawPointer {
+    UnsafeRawPointer(bitPattern: largeAddressBits)
+      ._unsafelyUnwrappedUnchecked
+  }
+
   @inlinable @inline(__always)
   internal var nativeUTF8Start: UnsafePointer<UInt8> {
     _internalInvariant(largeFastIsTailAllocated)
@@ -915,11 +933,12 @@ extension _StringObject {
     _internalInvariant(largeFastIsShared)
 #if _runtime(_ObjC)
     if largeIsCocoa {
-      return stableCocoaUTF8Pointer(cocoaObject)._unsafelyUnwrappedUnchecked
+      return withCocoaObject {
+        stableCocoaUTF8Pointer($0)._unsafelyUnwrappedUnchecked
+      }
     }
 #endif
-
-    return sharedStorage.start
+    return withSharedStorage { $0.start }
   }
 
   @usableFromInline
@@ -940,7 +959,24 @@ extension _StringObject {
     return _unsafeUncheckedDowncast(storage, to: __StringStorage.self)
 #else
     _internalInvariant(hasNativeStorage)
-    return Builtin.reinterpretCast(largeAddressBits)
+    let unmanaged = Unmanaged<__StringStorage>.fromOpaque(largeAddress)
+    return unmanaged.takeUnretainedValue()
+#endif
+  }
+
+  /// Call `body` with the native storage object of `self`, without retaining
+  /// it.
+  @inline(__always)
+  internal func withNativeStorage<R>(
+    _ body: (__StringStorage) -> R
+  ) -> R {
+#if arch(i386) || arch(arm) || arch(arm64_32) || arch(wasm32)
+    // FIXME: Do this properly.
+    return body(nativeStorage)
+#else
+    _internalInvariant(hasNativeStorage)
+    let unmanaged = Unmanaged<__StringStorage>.fromOpaque(largeAddress)
+    return unmanaged._withUnsafeGuaranteedRef { body($0) }
 #endif
   }
 
@@ -954,7 +990,23 @@ extension _StringObject {
 #else
     _internalInvariant(largeFastIsShared && !largeIsCocoa)
     _internalInvariant(hasSharedStorage)
-    return Builtin.reinterpretCast(largeAddressBits)
+    let unmanaged = Unmanaged<__SharedStringStorage>.fromOpaque(largeAddress)
+    return unmanaged.takeUnretainedValue()
+#endif
+  }
+
+  @inline(__always)
+  internal func withSharedStorage<R>(
+    _ body: (__SharedStringStorage) -> R
+  ) -> R {
+#if arch(i386) || arch(arm) || arch(arm64_32) || arch(wasm32)
+    // FIXME: Do this properly.
+    return body(sharedStorage)
+#else
+    _internalInvariant(largeFastIsShared && !largeIsCocoa)
+    _internalInvariant(hasSharedStorage)
+    let unmanaged = Unmanaged<__SharedStringStorage>.fromOpaque(largeAddress)
+    return unmanaged._withUnsafeGuaranteedRef { body($0) }
 #endif
   }
 
@@ -967,7 +1019,24 @@ extension _StringObject {
     return object
 #else
     _internalInvariant(largeIsCocoa && !isImmortal)
-    return Builtin.reinterpretCast(largeAddressBits)
+    let unmanaged = Unmanaged<AnyObject>.fromOpaque(largeAddress)
+    return unmanaged.takeUnretainedValue()
+#endif
+  }
+
+  /// Call `body` with the bridged Cocoa object in `self`, without retaining
+  /// it.
+  @inline(__always)
+  internal func withCocoaObject<R>(
+    _ body: (AnyObject) -> R
+  ) -> R {
+#if arch(i386) || arch(arm) || arch(arm64_32) || arch(wasm32)
+    // FIXME: Do this properly.
+    return body(cocoaObject)
+#else
+    _internalInvariant(largeIsCocoa && !isImmortal)
+    let unmanaged = Unmanaged<AnyObject>.fromOpaque(largeAddress)
+    return unmanaged._withUnsafeGuaranteedRef { body($0) }
 #endif
   }
 
@@ -976,7 +1045,8 @@ extension _StringObject {
   @inline(__always)
   internal var owner: AnyObject? {
     guard self.isMortal else { return nil }
-    return Builtin.reinterpretCast(largeAddressBits)
+    let unmanaged = Unmanaged<AnyObject>.fromOpaque(largeAddress)
+    return unmanaged.takeUnretainedValue()
   }
 }
 
@@ -1055,7 +1125,8 @@ extension _StringObject {
   @inline(__always)
   internal var objCBridgeableObject: AnyObject {
     _internalInvariant(hasObjCBridgeableObject)
-    return Builtin.reinterpretCast(largeAddressBits)
+    let unmanaged = Unmanaged<AnyObject>.fromOpaque(largeAddress)
+    return unmanaged.takeUnretainedValue()
   }
 
   // Whether the object provides fast UTF-8 contents that are nul-terminated
@@ -1219,7 +1290,8 @@ extension _StringObject {
         }
       }
       if _countAndFlags.isNativelyStored {
-        let anyObj = Builtin.reinterpretCast(largeAddressBits) as AnyObject
+        let unmanaged = Unmanaged<AnyObject>.fromOpaque(largeAddress)
+        let anyObj = unmanaged.takeUnretainedValue()
         _internalInvariant(anyObj is __StringStorage)
       }
     }

stdlib/public/core/StringStorage.swift

@@ -735,10 +735,11 @@ extension _StringGuts {
 
     let mutPtr: UnsafeMutablePointer<_StringBreadcrumbs?>
     if hasNativeStorage {
-      mutPtr = _object.nativeStorage._breadcrumbsAddress
+      mutPtr = _object.withNativeStorage { $0._breadcrumbsAddress }
     } else {
-      mutPtr = UnsafeMutablePointer(
-        Builtin.addressof(&_object.sharedStorage._breadcrumbs))
+      mutPtr = _object.withSharedStorage {
+        UnsafeMutablePointer(Builtin.addressof(&$0._breadcrumbs))
+      }
     }
 
     if let breadcrumbs = _stdlib_atomicAcquiringLoadARCRef(object: mutPtr) {

stdlib/public/core/StringUTF8View.swift

@@ -541,10 +541,10 @@ extension String.UTF8View {
 
     #if _runtime(_ObjC)
     // Currently, foreign means NSString
-    if let count = _cocoaStringUTF8Count(
-      _guts._object.cocoaObject,
-      range: i._encodedOffset ..< j._encodedOffset
-    ) {
+    let count = _guts._object.withCocoaObject {
+      _cocoaStringUTF8Count($0, range: i._encodedOffset ..< j._encodedOffset)
+    }
+    if let count {
       // _cocoaStringUTF8Count gave us the scalar aligned count, but we still
       // need to compensate for sub-scalar indexing, e.g. if `i` is in the
       // middle of a two-byte UTF8 scalar.