Skip to content

Fix crash: ConcurrentQueue mutates without a barrier - #38

Open
Kevin-4Spaces wants to merge 1 commit into
aptabase:mainfrom
Kevin-4Spaces:fix/concurrent-queue-mutation-barrier
Open

Fix crash: ConcurrentQueue mutates without a barrier#38
Kevin-4Spaces wants to merge 1 commit into
aptabase:mainfrom
Kevin-4Spaces:fix/concurrent-queue-mutation-barrier

Conversation

@Kevin-4Spaces

Copy link
Copy Markdown

The crash

Swift/Array.swift:1845: Fatal error: Array replace: subrange extends past the end

reached from AptabaseClient timer flush → EventDispatcher.flush()ConcurrentQueue.dequeue(count:).

Cause

ConcurrentQueue's queue is created with attributes: .concurrent, so a plain sync grants shared access — it does not serialize. enqueue(_:) and enqueue(contentsOf:) correctly use async(flags: .barrier), but dequeue() and dequeue(count:) mutate elements inside a plain sync:

queue.sync {
    for _ in 0 ..< min(count, self.elements.count) {
        dequeuedElements.append(self.elements.removeFirst())
    }
}

Two concurrent callers each evaluate self.elements.count, then both removeFirst(), and the second removes past the end.

This is reachable in normal use: flush() runs from the repeating timer and from app lifecycle handlers, so two flushes can overlap on a real device.

Fix

Give both dequeues sync(flags: .barrier). isEmpty and count keep the plain sync — concurrent reads are the reason for the concurrent queue.

Tests

Two regression tests drain a 2,000-element queue from 8 concurrent workers and assert every element is dequeued exactly once, covering both dequeue() and dequeue(count:).

Against the unpatched implementation both crash:

error: Exited with unexpected signal code 11

With the barrier, the full suite passes (11 tests, 0 failures).

`ConcurrentQueue` runs on a concurrent DispatchQueue, so a plain `sync`
grants shared (reader) access, not exclusive access. `enqueue` correctly
uses `async(flags: .barrier)`, but both `dequeue()` and `dequeue(count:)`
MUTATE `elements` inside a plain `sync` — so two callers can each read a
non-empty array and then `removeFirst()` past each other's end.

In production this crashes as:

    Swift/Array.swift:1845: Fatal error: Array replace: subrange extends past the end

reached from `EventDispatcher.flush()` → `ConcurrentQueue.dequeue(count:)`.
`flush()` is driven by a repeating timer and also by app lifecycle events,
so overlapping flushes happen on real devices.

Fix: give both dequeues `sync(flags: .barrier)`. The read-only `isEmpty`
and `count` keep the plain `sync` — concurrent reads are the point of the
concurrent queue.

Adds two regression tests that drain the queue from 8 concurrent workers
and assert every element comes out exactly once. Both crash with SIGSEGV
against the unpatched implementation and pass with the barrier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants