From 7a0e151b89f27babcfae6ab833d0fe965dea1bf5 Mon Sep 17 00:00:00 2001 From: Jeff Verkoeyen Date: Wed, 16 Oct 2024 21:34:49 -0700 Subject: [PATCH 1/3] Catch race condition in PostHogFileBackedQueue.deleteFiles In rare cases, `deleteFiles(_:)` can be invoked such that, between the check for items.isEmpty and the call to items.remove, the task gets pre-empted and items is mutated. Once the thread resumes, the `.remove(at: 0)` operation causes a crash. --- PostHog/PostHogFileBackedQueue.swift | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/PostHog/PostHogFileBackedQueue.swift b/PostHog/PostHogFileBackedQueue.swift index 24adf0582..3eb09c476 100644 --- a/PostHog/PostHogFileBackedQueue.swift +++ b/PostHog/PostHogFileBackedQueue.swift @@ -100,9 +100,14 @@ class PostHogFileBackedQueue { private func deleteFiles(_ count: Int) { for _ in 0 ..< count { - if items.isEmpty { return } - let removed = items.remove(at: 0) // We always remove from the top of the queue - + guard let removed: String = _items.mutate({ items in + if items.isEmpty { + return nil + } + return items.remove(at: 0) // We always remove from the top of the queue + }) else { + continue + } deleteSafely(queue.appendingPathComponent(removed)) } } From 5324f29109f110b46f901609de92f4e942e8e160 Mon Sep 17 00:00:00 2001 From: Jeff Verkoeyen Date: Thu, 17 Oct 2024 04:41:01 +0000 Subject: [PATCH 2/3] Update ReadWriteLock. --- PostHog/Utils/ReadWriteLock.swift | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/PostHog/Utils/ReadWriteLock.swift b/PostHog/Utils/ReadWriteLock.swift index cd7d05934..afdfed634 100644 --- a/PostHog/Utils/ReadWriteLock.swift +++ b/PostHog/Utils/ReadWriteLock.swift @@ -50,10 +50,11 @@ public final class ReadWriteLock { /// The lock will be acquired once for writing before invoking the closure. /// /// - Parameter closure: The closure with the mutable value. - public func mutate(_ closure: (inout Value) -> Void) { + public func mutate(_ closure: (inout Value) -> T) -> T { pthread_rwlock_wrlock(&rwlock) - closure(&value) + let result = closure(&value) pthread_rwlock_unlock(&rwlock) + return result } } From bbcbf8b47ecbb94ab0365190596fe0241532883a Mon Sep 17 00:00:00 2001 From: Jeff Verkoeyen Date: Thu, 17 Oct 2024 16:27:07 +0000 Subject: [PATCH 3/3] Address feedback. --- PostHog/PostHogFileBackedQueue.swift | 7 +++---- PostHog/Utils/ReadWriteLock.swift | 8 +++++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/PostHog/PostHogFileBackedQueue.swift b/PostHog/PostHogFileBackedQueue.swift index 3eb09c476..ab9160709 100644 --- a/PostHog/PostHogFileBackedQueue.swift +++ b/PostHog/PostHogFileBackedQueue.swift @@ -100,15 +100,14 @@ class PostHogFileBackedQueue { private func deleteFiles(_ count: Int) { for _ in 0 ..< count { - guard let removed: String = _items.mutate({ items in + if let removed: String = _items.mutate({ items in if items.isEmpty { return nil } return items.remove(at: 0) // We always remove from the top of the queue - }) else { - continue + }) { + deleteSafely(queue.appendingPathComponent(removed)) } - deleteSafely(queue.appendingPathComponent(removed)) } } } diff --git a/PostHog/Utils/ReadWriteLock.swift b/PostHog/Utils/ReadWriteLock.swift index afdfed634..c782a15f0 100644 --- a/PostHog/Utils/ReadWriteLock.swift +++ b/PostHog/Utils/ReadWriteLock.swift @@ -50,11 +50,13 @@ public final class ReadWriteLock { /// The lock will be acquired once for writing before invoking the closure. /// /// - Parameter closure: The closure with the mutable value. + @discardableResult public func mutate(_ closure: (inout Value) -> T) -> T { pthread_rwlock_wrlock(&rwlock) - let result = closure(&value) - pthread_rwlock_unlock(&rwlock) - return result + defer { + pthread_rwlock_unlock(&rwlock) + } + return closure(&value) } }