[FirebaseCoreInternal] Guard against invalid indexing into RingBuffer type (#10032)

* [FirebaseCoreInternal] Refactor RingBuffer indexing code

* Add unit tests for pop API

* Some more refactoring

* Add bandaid for improper bounds

* Review

* Review 2

* Prevent duplicate dailies

* CHANGELOG

* Use guard statement instead

* Collect more info

Author: ncooke3

FirebaseCore/CHANGELOG.md

@@ -1,3 +1,6 @@
+# Firebase 9.4.0
+- [fixed] Fixed rare crash on launch due to out-of-bounds exception in FirebaseCore. (#10025)
+
 # Firebase 9.3.0
 - [fixed] Remove GoogleSignInSwiftSupport from Zip and Carthage distributions due to
   infeasibility. The GoogleSignIn distribution continues. (#9937)

FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatsBundle.swift

@@ -57,18 +57,49 @@ struct HeartbeatsBundle: Codable, HeartbeatsPayloadConvertible {
       return // Do not append if capacity is non-positive.
     }
 
-    // 1. Push the heartbeat to the back of the buffer.
-    if let overwrittenHeartbeat = buffer.push(heartbeat) {
-      // If a heartbeat was overwritten, update the cache to ensure it's date
-      // is removed (if it was stored).
-      lastAddedHeartbeatDates = lastAddedHeartbeatDates.mapValues { date in
-        overwrittenHeartbeat.date == date ? .distantPast : date
+    do {
+      // Push the heartbeat to the back of the buffer.
+      if let overwrittenHeartbeat = try buffer.push(heartbeat) {
+        // If a heartbeat was overwritten, update the cache to ensure it's date
+        // is removed.
+        lastAddedHeartbeatDates = lastAddedHeartbeatDates.mapValues { date in
+          overwrittenHeartbeat.date == date ? .distantPast : date
+        }
+      }
+
+      // Update cache with the new heartbeat's date.
+      heartbeat.timePeriods.forEach {
+        lastAddedHeartbeatDates[$0] = heartbeat.date
       }
-    }
 
-    // 2. Update cache with the new heartbeat's date.
-    heartbeat.timePeriods.forEach {
-      lastAddedHeartbeatDates[$0] = heartbeat.date
+    } catch let error as RingBufferError {
+      // A ring buffer error occurred while pushing to the buffer so the bundle
+      // is reset.
+      self = HeartbeatsBundle(capacity: capacity)
+
+      // Create a diagnostic heartbeat to capture the failure and add it to the
+      // buffer. The failure is added as a key/value pair to the agent string.
+      // Given that the ring buffer has been reset, it is not expected for the
+      // second push attempt to fail.
+      let errorDescription = error.errorDescription.replacingOccurrences(of: " ", with: "-")
+      let diagnosticHeartbeat = Heartbeat(
+        agent: "\(heartbeat.agent) error/\(errorDescription)",
+        date: heartbeat.date,
+        timePeriods: heartbeat.timePeriods
+      )
+
+      let secondPushAttempt = Result {
+        try buffer.push(diagnosticHeartbeat)
+      }
+
+      if case .success = secondPushAttempt {
+        // Update cache with the new heartbeat's date.
+        diagnosticHeartbeat.timePeriods.forEach {
+          lastAddedHeartbeatDates[$0] = diagnosticHeartbeat.date
+        }
+      }
+    } catch {
+      // Ignore other error.
     }
   }
 
@@ -90,7 +121,11 @@ struct HeartbeatsBundle: Codable, HeartbeatsPayloadConvertible {
     }
 
     poppedHeartbeats.reversed().forEach {
-      buffer.push($0)
+      do {
+        try buffer.push($0)
+      } catch {
+        // Ignore error.
+      }
     }
 
     return removedHeartbeat

FirebaseCore/Internal/Sources/HeartbeatLogging/RingBuffer.swift

@@ -14,38 +14,61 @@
 
 import Foundation
 
+/// Error types for `RingBuffer` operations.
+enum RingBufferError: LocalizedError {
+  case outOfBoundsPush(pushIndex: Array.Index, endIndex: Array.Index)
+
+  var errorDescription: String {
+    switch self {
+    case let .outOfBoundsPush(pushIndex, endIndex):
+      return "Out-of-bounds push at index \(pushIndex) to ring buffer with" +
+        "end index of \(endIndex)."
+    }
+  }
+}
+
 /// A generic circular queue structure.
 struct RingBuffer<Element>: Sequence {
   /// An array of heartbeats treated as a circular queue and intialized with a fixed capacity.
   private var circularQueue: [Element?]
   /// The current "tail" and insert point for the `circularQueue`.
-  private var tailIndex: Int = 0
+  private var tailIndex: Array.Index
 
   /// Designated initializer.
   /// - Parameter capacity: An `Int` representing the capacity.
   init(capacity: Int) {
     circularQueue = Array(repeating: nil, count: capacity)
+    tailIndex = circularQueue.startIndex
   }
 
   /// Pushes an element to the back of the buffer, returning the element (`Element?`) that was overwritten.
   /// - Parameter element: The element to push to the back of the buffer.
   /// - Returns: The element that was overwritten or `nil` if nothing was overwritten.
   /// - Complexity: O(1)
   @discardableResult
-  mutating func push(_ element: Element) -> Element? {
+  mutating func push(_ element: Element) throws -> Element? {
     guard circularQueue.count > 0 else {
       // Do not push if `circularQueue` is a fixed empty array.
       return nil
     }
 
-    defer {
-      // Increment index, wrapping around to the start if needed.
-      tailIndex += 1
-      tailIndex %= circularQueue.count
+    guard circularQueue.indices.contains(tailIndex) else {
+      // We have somehow entered an invalid state (#10025).
+      throw RingBufferError.outOfBoundsPush(
+        pushIndex: tailIndex,
+        endIndex: circularQueue.endIndex
+      )
     }
 
     let replaced = circularQueue[tailIndex]
     circularQueue[tailIndex] = element
+
+    // Increment index, wrapping around to the start if needed.
+    tailIndex += 1
+    if tailIndex >= circularQueue.endIndex {
+      tailIndex = circularQueue.startIndex
+    }
+
     return replaced
   }
 
@@ -61,8 +84,8 @@ struct RingBuffer<Element>: Sequence {
 
     // Decrement index, wrapping around to the back if needed.
     tailIndex -= 1
-    if tailIndex < 0 {
-      tailIndex = circularQueue.count - 1
+    if tailIndex < circularQueue.startIndex {
+      tailIndex = circularQueue.endIndex - 1
     }
 
     guard let popped = circularQueue[tailIndex] else {

FirebaseCore/Internal/Tests/Unit/RingBufferTests.swift

@@ -23,7 +23,7 @@ class RingBufferTests: XCTestCase {
     // Given
     var ringBuffer = RingBuffer<Element>(capacity: 0)
     // When
-    ringBuffer.push("ezra")
+    try ringBuffer.push("ezra")
     // Then
     XCTAssertEqual(Array(ringBuffer), [])
   }
@@ -32,17 +32,17 @@ class RingBufferTests: XCTestCase {
     // Given
     var ringBuffer = RingBuffer<Element>(capacity: 3) // [nil, nil, nil]
     // When
-    let overwrittenElement = ringBuffer.push("vader") // ["vader", nil, nil]
+    let overwrittenElement = try ringBuffer.push("vader") // ["vader", nil, nil]
     // Then
     XCTAssertNil(overwrittenElement)
   }
 
   func testPush_WhenAtFullCapacity_OverwritesAndReturnsTailElement() throws {
     // Given
     var ringBuffer = RingBuffer<Element>(capacity: 1) // [nil]
-    ringBuffer.push("luke") // ["luke"] where "luke" is the tail element.
+    try ringBuffer.push("luke") // ["luke"] where "luke" is the tail element.
     // When
-    let overwrittenElement = ringBuffer.push("vader")
+    let overwrittenElement = try ringBuffer.push("vader")
     // Then
     XCTAssertEqual(overwrittenElement, "luke")
     XCTAssertEqual(Array(ringBuffer), ["vader"])
@@ -52,10 +52,10 @@ class RingBufferTests: XCTestCase {
     // Given
     var ringBuffer = RingBuffer<Element>(capacity: 3) // [nil, nil, nil]
     // When
-    ringBuffer.push("chewy") // ["chewy", nil, nil]
-    ringBuffer.push("vader") // ["chewy", "vader", nil]
-    ringBuffer.push("jabba") // ["chewy", "vader", "jabba"]
-    ringBuffer.push("lando") // ["lando", "vader", "jabba"]
+    try ringBuffer.push("chewy") // ["chewy", nil, nil]
+    try ringBuffer.push("vader") // ["chewy", "vader", nil]
+    try ringBuffer.push("jabba") // ["chewy", "vader", "jabba"]
+    try ringBuffer.push("lando") // ["lando", "vader", "jabba"]
     // Then
     XCTAssertEqual(Array(ringBuffer), ["lando", "vader", "jabba"])
   }
@@ -64,9 +64,9 @@ class RingBufferTests: XCTestCase {
     // Given
     var ringBuffer = RingBuffer<Element>(capacity: 10)
     // When
-    ringBuffer.push("han solo")
-    ringBuffer.push("boba")
-    ringBuffer.push("jabba")
+    try ringBuffer.push("han solo")
+    try ringBuffer.push("boba")
+    try ringBuffer.push("jabba")
     // Then
     XCTAssertEqual(Array(ringBuffer), ["han solo", "boba", "jabba"])
   }
@@ -76,12 +76,34 @@ class RingBufferTests: XCTestCase {
     var ringBuffer = RingBuffer<Int>(capacity: 10)
     // When
     for index in 1 ... 1000 {
-      ringBuffer.push(index)
+      try ringBuffer.push(index)
     }
     // Then
     XCTAssertEqual(Array(ringBuffer), Array(991 ... 1000))
   }
 
+  func testPopBeforePushing() throws {
+    // Given
+    var ringBuffer = RingBuffer<Element>(capacity: 2) // [`tailIndex` -> nil, nil]
+    // When
+    ringBuffer.pop() // [nil, `tailIndex` -> nil]
+    try ringBuffer.push("yoda") // [nil, "yoda"]
+    try ringBuffer.push("mando") // ["mando", "yoda"]
+    // Then
+    XCTAssertEqual(Array(ringBuffer), ["mando", "yoda"])
+  }
+
+  func testPopStressTest() throws {
+    // Given
+    var ringBuffer = RingBuffer<Int>(capacity: 10)
+    // When
+    for _ in 1 ... 1000 {
+      XCTAssertNil(ringBuffer.pop())
+    }
+    // Then
+    XCTAssertEqual(Array(ringBuffer), [])
+  }
+
   func testPop_WhenCapacityIsZero_DoesNothingAndReturnsNil() throws {
     // Given
     var ringBuffer = RingBuffer<String>(capacity: 0)
@@ -95,9 +117,9 @@ class RingBufferTests: XCTestCase {
   func testPopRemovesAndReturnsLastElement() throws {
     // Given
     var ringBuffer = RingBuffer<Element>(capacity: 3)
-    ringBuffer.push("one")
-    ringBuffer.push("two")
-    ringBuffer.push("three")
+    try ringBuffer.push("one")
+    try ringBuffer.push("two")
+    try ringBuffer.push("three")
     // When
     XCTAssertEqual(ringBuffer.pop(), "three")
     XCTAssertEqual(ringBuffer.pop(), "two")
@@ -110,7 +132,7 @@ class RingBufferTests: XCTestCase {
     // Given
     var ringBuffer = RingBuffer<Int>(capacity: 10)
     for number in Array(1 ... 15) {
-      ringBuffer.push(number)
+      try ringBuffer.push(number)
     }
 
     // ringBuffer: [11, 12, 13, 14, 15, 6, 7, 8, 9, 10]