[stdlib] Review and fix some problems with unsafe buffer and Range initialization

stdlib/public/core/ClosedRange.swift

@@ -68,6 +68,12 @@ public struct ClosedRange<Bound: Comparable> {
   /// The range's upper bound.
   public let upperBound: Bound
 
+  @_alwaysEmitIntoClient @inline(__always)
+  internal init(_uncheckedBounds bounds: (lower: Bound, upper: Bound)) {
+    self.lowerBound = bounds.lower
+    self.upperBound = bounds.upper
+  }
+
   /// Creates an instance with the given bounds.
   ///
   /// Because this initializer does not perform any checks, it should be used
@@ -78,8 +84,9 @@ public struct ClosedRange<Bound: Comparable> {
   /// - Parameter bounds: A tuple of the lower and upper bounds of the range.
   @inlinable
   public init(uncheckedBounds bounds: (lower: Bound, upper: Bound)) {
-    self.lowerBound = bounds.lower
-    self.upperBound = bounds.upper
+    _debugPrecondition(bounds.lower <= bounds.upper,
+      "ClosedRange requires lowerBound <= upperBound")
+    self.init(_uncheckedBounds: (lower: bounds.lower, upper: bounds.upper))
   }
 }
 
@@ -336,7 +343,7 @@ extension Comparable {
   public static func ... (minimum: Self, maximum: Self) -> ClosedRange<Self> {
     _precondition(
       minimum <= maximum, "Range requires lowerBound <= upperBound")
-    return ClosedRange(uncheckedBounds: (lower: minimum, upper: maximum))
+    return ClosedRange(_uncheckedBounds: (lower: minimum, upper: maximum))
   }
 }
 

stdlib/public/core/Range.swift

@@ -157,6 +157,12 @@ public struct Range<Bound: Comparable> {
   /// instance does not contain its upper bound.
   public let upperBound: Bound
 
+  @_alwaysEmitIntoClient @inline(__always)
+  internal init(_uncheckedBounds bounds: (lower: Bound, upper: Bound)) {
+    self.lowerBound = bounds.lower
+    self.upperBound = bounds.upper
+  }
+
   /// Creates an instance with the given bounds.
   ///
   /// Because this initializer does not perform any checks, it should be used
@@ -167,8 +173,9 @@ public struct Range<Bound: Comparable> {
   /// - Parameter bounds: A tuple of the lower and upper bounds of the range.
   @inlinable
   public init(uncheckedBounds bounds: (lower: Bound, upper: Bound)) {
-    self.lowerBound = bounds.lower
-    self.upperBound = bounds.upper
+    _debugPrecondition(bounds.lower <= bounds.upper,
+      "Range requires lowerBound <= upperBound")
+    self.init(_uncheckedBounds: (lower: bounds.lower, upper: bounds.upper))
   }
 
   /// Returns a Boolean value indicating whether the given element is contained
@@ -729,7 +736,7 @@ extension Comparable {
   public static func ..< (minimum: Self, maximum: Self) -> Range<Self> {
     _precondition(minimum <= maximum,
       "Range requires lowerBound <= upperBound")
-    return Range(uncheckedBounds: (lower: minimum, upper: maximum))
+    return Range(_uncheckedBounds: (lower: minimum, upper: maximum))
   }
 
   /// Returns a partial range up to, but not including, its upper bound.

stdlib/public/core/String.swift

@@ -458,6 +458,7 @@ extension String {
       contigBytes._providesContiguousBytesNoCopy
     {
       self = contigBytes.withUnsafeBytes { rawBufPtr in
+        Builtin.onFastPath() // encourage SIL Optimizer to inline this closure
         return String._fromUTF8Repairing(
           UnsafeBufferPointer(
             start: rawBufPtr.baseAddress?.assumingMemoryBound(to: UInt8.self),

stdlib/public/core/UnsafeBufferPointer.swift.gyb

@@ -207,7 +207,14 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle
     // optimizer is not capable of creating partial specializations yet.
     // NOTE: Range checks are not performed here, because it is done later by
     // the subscript function.
-    return end - start
+    // NOTE: We allow the subtraction to silently overflow in release builds
+    // to eliminate a superflous check when `start` and `end` are both valid
+    // indices. (The operation can only overflow if `start` is negative, which
+    // implies it's an invalid index.) `Collection` does not specify what
+    // `distance` should return when given an invalid index pair.
+    let result = end.subtractingReportingOverflow(start)
+    _debugPrecondition(!result.overflow)
+    return result.partialValue
   }
 
   @inlinable // unsafe-performance
@@ -401,9 +408,9 @@ extension Unsafe${Mutable}BufferPointer {
   public init(
     @_nonEphemeral start: Unsafe${Mutable}Pointer<Element>?, count: Int
   ) {
-    _precondition(
+    _debugPrecondition(
       count >= 0, "Unsafe${Mutable}BufferPointer with negative count")
-    _precondition(
+    _debugPrecondition(
       count == 0 || start != nil,
       "Unsafe${Mutable}BufferPointer has a nil start and nonzero count")
     _position = start
@@ -498,6 +505,17 @@ extension Unsafe${Mutable}BufferPointer {
   /// - Parameter slice: The buffer slice to rebase.
   @inlinable // unsafe-performance
   public init(rebasing slice: Slice<UnsafeBufferPointer<Element>>) {
+    // NOTE: `Slice` does not guarantee that its start/end indices are valid
+    // in `base` -- it merely ensures that `startIndex <= endIndex`.
+    // We need manually check that we aren't given an invalid slice,
+    // or the resulting collection would allow access that was
+    // out-of-bounds with respect to the original base buffer.
+    // We only do this in debug builds to prevent a measurable performance
+    // degradation wrt passing around pointers not wrapped in a BufferPointer
+    // construct.
+    _debugPrecondition(
+      slice.startIndex >= 0 && slice.endIndex <= slice.base.count,
+      "Invalid slice")
     let base = slice.base.baseAddress?.advanced(by: slice.startIndex)
     let count = slice.endIndex &- slice.startIndex
     self.init(start: base, count: count)

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

@@ -253,7 +253,7 @@ extension Unsafe${Mutable}RawBufferPointer: ${Mutable}Collection {
   @inlinable
   public var count: Int {
     if let pos = _position {
-      return _end! - pos
+      return _assumeNonNegative(_end! - pos)
     }
     return 0
   }
@@ -432,8 +432,8 @@ extension Unsafe${Mutable}RawBufferPointer {
   public init(
     @_nonEphemeral start: Unsafe${Mutable}RawPointer?, count: Int
   ) {
-    _precondition(count >= 0, "${Self} with negative count")
-    _precondition(count == 0 || start != nil,
+    _debugPrecondition(count >= 0, "${Self} with negative count")
+    _debugPrecondition(count == 0 || start != nil,
       "${Self} has a nil start and nonzero count")
     _position = start
     _end = start.map { $0 + count }
@@ -511,6 +511,17 @@ extension Unsafe${Mutable}RawBufferPointer {
   /// - Parameter slice: The raw buffer slice to rebase.
   @inlinable
   public init(rebasing slice: Slice<UnsafeRawBufferPointer>) {
+    // NOTE: `Slice` does not guarantee that its start/end indices are valid
+    // in `base` -- it merely ensures that `startIndex <= endIndex`.
+    // We need manually check that we aren't given an invalid slice,
+    // or the resulting collection would allow access that was
+    // out-of-bounds with respect to the original base buffer.
+    // We only do this in debug builds to prevent a measurable performance
+    // degradation wrt passing around pointers not wrapped in a BufferPointer
+    // construct.
+    _debugPrecondition(
+      slice.startIndex >= 0 && slice.endIndex <= slice.base.count,
+      "Invalid slice")
     let base = slice.base.baseAddress?.advanced(by: slice.startIndex)
     let count = slice.endIndex &- slice.startIndex
     self.init(start: base, count: count)

test/stdlib/RangeTraps.swift

@@ -116,5 +116,23 @@ RangeTraps.test("throughNaN")
   _ = ...Double.nan
 }
 
+RangeTraps.test("UncheckedHalfOpen")
+  .xfail(.custom(
+    { !_isDebugAssertConfiguration() },
+    reason: "assertions are disabled in Release and Unchecked mode"))
+  .code {
+    expectCrashLater()
+    var range = Range(uncheckedBounds: (lower: 1, upper: 0))
+  }
+
+RangeTraps.test("UncheckedClosed")
+  .xfail(.custom(
+    { !_isDebugAssertConfiguration() },
+    reason: "assertions are disabled in Release and Unchecked mode"))
+  .code {
+    expectCrashLater()
+    var range = ClosedRange(uncheckedBounds: (lower: 1, upper: 0))
+  }
+
 runAllTests()
 

validation-test/stdlib/ArrayTraps.swift.gyb

@@ -172,8 +172,8 @@ ${ArrayTy}Traps.test("${'remove(at:)/%s' % index}")
 
 ArrayTraps.test("unsafeLength")
   .skip(.custom(
-    { _isFastAssertConfiguration() },
-    reason: "this trap is not guaranteed to happen in -Ounchecked"))
+    { !_isDebugAssertConfiguration() },
+    reason: "this trap is not guaranteed to happen in -O or -Ounchecked"))
   .crashOutputMatches(_isDebugAssertConfiguration() ?
     "UnsafeBufferPointer with negative count" : "")
   .code {

validation-test/stdlib/Range.swift.gyb

@@ -335,17 +335,31 @@ extension ${Self} where Bound : TestProtocol1 {
 var ${TestSuite} = TestSuite("${Self}")
 
 ${TestSuite}.test("init(uncheckedBounds:)")
-  .forEach(in: [(1, 2), (1, 1), (2, 1)]) {
+  .forEach(in: [(1, 2), (1, 1)]) {
   (lowerInt, upperInt) in
 
-  // Check that 'init(uncheckedBounds:)' does not perform precondition checks,
-  // allowing to create ranges that break invariants.
   let r = ${Self}(
     uncheckedBounds: (lower: ${Bound}(lowerInt), upper: ${Bound}(upperInt)))
   expectEqual(lowerInt, r.lowerBound.value)
   expectEqual(upperInt, r.upperBound.value)
 }
 
+${TestSuite}.test("init(uncheckedBounds:) with invalid values")
+  .xfail(
+    .custom(
+      { _isDebugAssertConfiguration() },
+      reason: "unchecked initializer still checks its input in Debug mode"))
+  .code {
+  let low = 2
+  let up = 1
+  // Check that 'init(uncheckedBounds:)' does not perform precondition checks,
+  // allowing to create ranges that break invariants.
+  let r = ${Self}(
+    uncheckedBounds: (lower: ${Bound}(low), upper: ${Bound}(up)))
+  expectEqual(low, r.lowerBound.value)
+  expectEqual(up, r.upperBound.value)
+}
+
 %   for (DestinationSelf, _, _, _) in all_range_types:
 ${TestSuite}.test("init(${DestinationSelf})/whereBoundIsStrideable")
   .forEach(in: [(0, 0), (1, 2), (10, 20), (Int.min, Int.max)]) {