# A04 — threadSafeQueue: queue->first and node->element read outside the mutex

Bug ref      : always.md A.4 ; pharo.md §3.21
Severity     : HIGH (use-after-free on every concurrent dequeue under FFI workload)
File         : src/threadSafeQueue/threadSafeQueue.c
Lines (HEAD) : 113-137 (`threadsafe_queue_take`)

## Problem

```c
void *threadsafe_queue_take(TSQueue *queue) {
    if (queue->semaphore->wait(queue->semaphore) != 0){
        ...
        return NULL;
    }

    TSQueueNode *node = queue->first;          // READ outside mutex

    if(node == NULL)
        return NULL;

    void *element = node->element;             // READ outside mutex

    platform_semaphore_wait(queue->mutex);
    if (queue->first == queue->last) {
        queue->first = NULL;
        queue->last = NULL;
    } else {
        queue->first = node->next;
    }
    platform_semaphore_signal(queue->mutex);
    free(node);                                // free under mutex
    return element;
}
```

Two consumers can both pass the counting semaphore and execute the
unsynchronised `queue->first` read. Each then takes the mutex in
turn; the first consumer reseats `queue->first` and `free(node)`s the
old head. When the second consumer's mutex critical section runs
later (with its locally cached `node` still pointing at the freed
memory), it reads `node->next` from freed memory.

The hazard is reachable from every FFI workload: the FFI worker queue
and callback queue are both built on `__TSQueue`.

## Fix

Move the entire dequeue — head fetch, NULL check, element read,
link update, free — inside the mutex. Take the mutex first; only
then look at `queue->first`. The counting semaphore continues to
provide back-pressure outside the critical section.

```diff
diff --git a/src/threadSafeQueue/threadSafeQueue.c b/src/threadSafeQueue/threadSafeQueue.c
index 3f70305a7..cc155d174 100644
--- a/src/threadSafeQueue/threadSafeQueue.c
+++ b/src/threadSafeQueue/threadSafeQueue.c
@@ -117,21 +117,28 @@ void *threadsafe_queue_take(TSQueue *queue) {
 		return NULL;
 	}
 
-	TSQueueNode *node = queue->first;
+	platform_semaphore_wait(queue->mutex);
 
-	if(node == NULL)
+	TSQueueNode *node = queue->first;
+	if(node == NULL) {
+		/* Counting semaphore was signalled but the queue is empty
+		 * (can happen on shutdown when threadsafe_queue_free has
+		 * run concurrently). Release the mutex and report empty. */
+		platform_semaphore_signal(queue->mutex);
 		return NULL;
+	}
 
 	void *element = node->element;
 
-	platform_semaphore_wait(queue->mutex);
 	if (queue->first == queue->last) {
 		queue->first = NULL;
 		queue->last = NULL;
 	} else {
 		queue->first = node->next;
 	}
-	platform_semaphore_signal(queue->mutex);
 	free(node);
+
+	platform_semaphore_signal(queue->mutex);
+
 	return element;
 }

```

## Test plan

- Run 8 producer / 8 consumer threads on a single queue with 10⁷
  enqueue/dequeue pairs under TSAN. Before: TSAN reports a race on
  `queue->first` and `node->element`. After: clean.
- Run the FFI callback test suite from `ffiTestLibrary` under TSAN
  with `--repeat=10` to catch transient races.

## Risk notes

- Moves the `free(node)` inside the mutex. The critical section grows
  by one free() — typically nanoseconds.
- Behaviour on shutdown (counting semaphore signalled but queue
  drained by a concurrent `threadsafe_queue_free`) now returns NULL
  rather than dereferencing nothing-allocated memory.
- No API change.