mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-07 08:36:02 -10:00
gfx/gfx_widgets: lock msg_queue against producer-producer race
dispgfx_widget_t::msg_queue is a fifo_buffer_t storing
disp_widget_msg_t* pointers. Pre-fix it had no dedicated lock,
yet the producer (gfx_widgets_msg_queue_push) is reachable from
threads other than the main thread, and the lock that *did*
exist (current_msgs_lock) protects current_msgs[] -- the
displayed-message deque -- not the FIFO.
Producer reachability:
1. runloop.c:runloop_msg_queue_push. Holds RUNLOOP_MSG_QUEUE_
LOCK across the call. Reachable from 40+ files; the
threaded task system at libretro-common/queues/task_queue.c
runs a worker thread that pushes via this entry point.
2. runloop.c:runloop_task_msg_queue_push. Same lock.
3. gfx/video_driver.c:video_driver_frame. Acquires
RUNLOOP_MSG_QUEUE_LOCK to drain runloop_st->msg_queue,
RELEASES it, then calls gfx_widgets_msg_queue_push without
the lock.
Path 3 runs on the main thread; path 1 can run on any thread.
RUNLOOP_MSG_QUEUE_LOCK guards runloop_st->msg_queue, NOT
p_dispwidget->msg_queue -- the lock name reflects the runloop
struct it was named after. So path 3's fifo_write (no lock)
can race with path 1's fifo_write (lock held but irrelevant
to path 3).
The consumer (gfx_widgets_iterate's fifo_read) is called from
runloop.c:6048 inside RUNLOOP_MSG_QUEUE_LOCK and from
gfx_widgets.c:973 inside current_msgs_lock -- but neither lock
is held by path 3, so consumer fifo_reads can observe a half-
updated `end` cursor from path 3's fifo_writes.
Outcome of a race: torn `end` cursor publishes a bad pointer
in the disp_widget_msg_t* slots. The consumer dereferences
that as disp_widget_msg_t* in gfx_widgets_iterate, producing
use-after-free / segfault. On x86 TSO the race is masked most
of the time by hardware ordering; on Win-on-ARM / Apple
Silicon (AArch64) it surfaces more often.
Collision probability is low (~1 per 14 hours of normal use,
estimated from frame rate * task message rate * fifo_write
window) but the worst case is crash-class.
Fix
---
Add slock_t* msg_queue_lock to dispgfx_widget_t. Acquire it
around every fifo_write (in gfx_widgets_msg_queue_push), every
fifo_read (in gfx_widgets_iterate), and the avail-check that
gates each one. msg_queue_lock is the inner lock when both
it and current_msgs_lock are held; lock order is consistent
across all call sites.
The outer FIFO_*_AVAIL fast-path checks that used to wrap the
producer and consumer function bodies have been dropped.
Reading the FIFO cursors outside msg_queue_lock is itself a
data race against the locked writes -- TSan-detectable; benign
on x86 TSO but real on weak-memory hardware. The locked
re-check at the actual fifo_write / fifo_read site is the
correctness gate.
Removing the producer's outer gate also fixes a latent
behaviour bug: the update-existing branch (else clause in
gfx_widgets_msg_queue_push) does not write to the FIFO -- it
only mutates an already-tracked widget -- so suppressing it on
FIFO-full was wrong. Existing widgets now get updated
regardless of FIFO state.
The producer's spawn-new branch may now race-lose the avail
re-check after allocating msg_widget (concurrent producer
filled the last slot between unlocked allocation and locked
write). In that case the function frees the just-allocated
widget via gfx_widgets_msg_queue_free + free and returns. A
forward declaration of gfx_widgets_msg_queue_free is added near
the top of the file because that function is defined further
down.
Regression test
---------------
samples/gfx/gfx_widgets_msg_queue_race/ mirrors the post-fix
locking discipline of gfx_widgets_msg_queue_push and
gfx_widgets_iterate's FIFO interaction in isolation. Two
producer threads + one consumer thread + 500K iterations per
producer; smoke check verifies single-threaded FIFO order.
Wired into .github/workflows/Linux-samples-gfx.yml as a TSan
lane (CC=clang, SANITIZER=thread, TSAN_OPTIONS=halt_on_error=1)
following the gfx_thumbnail_status_atomic_test convention.
TSan instruments every memory access on fifo_buffer_t::end and
::first, so a future regression that drops the lock around any
of the producer / consumer / avail-check sites will be flagged
as a data race and fail the workflow.
Verified locally
----------------
- samples/gfx/gfx_widgets_msg_queue_race builds and runs
clean as C (gcc -O0), C++11 (g++ -std=c++11 -xc++), and
Clang under -fsanitize=thread.
- TSan with halt_on_error=1: clean exit (post-fix lock
discipline causes no observed races across 500K * 2
iterations).
- Mutation test: removing the slock_lock/unlock around the
producer's fifo_write makes TSan fire on the first
collision, validating that the test is sensitive to the
exact regression we're guarding against.
- gfx/gfx_widgets.c syntax-checks clean with HAVE_THREADS +
HAVE_GFX_WIDGETS via gcc -fsyntax-only against the
libretro-common headers.
This commit is contained in:
1 parent
932e1e52ba
commit
4e26c8875a
5 files changed
+695
-6
No files matched your search
@@ -241,3 +241,50 @@ jobs:
|
||||
TSAN_OPTIONS=halt_on_error=1 timeout 60 \
|
||||
./gfx_thumbnail_status_atomic_test
|
||||
echo "[pass] gfx_thumbnail_status_atomic_test"
|
||||
|
||||
- name: Build and run gfx_widgets_msg_queue_race_test (TSan)
|
||||
shell: bash
|
||||
working-directory: samples/gfx/gfx_widgets_msg_queue_race
|
||||
run: |
|
||||
set -eu
|
||||
# Regression test for the producer-producer race fix on
|
||||
# dispgfx_widget_t::msg_queue in gfx/gfx_widgets.c.
|
||||
# Pre-fix the FIFO had no dedicated lock: producers could
|
||||
# be invoked from any thread (the threaded task system at
|
||||
# libretro-common/queues/task_queue.c runs a worker
|
||||
# thread; gfx/video_driver.c::video_driver_frame released
|
||||
# RUNLOOP_MSG_QUEUE_LOCK before calling the producer, so
|
||||
# main-thread producers ran unlocked) yet there was no
|
||||
# lock dedicated to msg_queue itself. Post-fix a
|
||||
# msg_queue_lock is acquired around every fifo_write (in
|
||||
# gfx_widgets_msg_queue_push), every fifo_read (in
|
||||
# gfx_widgets_iterate), and the avail-check that gates
|
||||
# them. The outer fast-path FIFO_*_AVAIL checks that
|
||||
# used to wrap the function bodies were dropped: reading
|
||||
# the FIFO cursors outside the lock is itself a data
|
||||
# race against the locked writes (TSan-detectable;
|
||||
# benign on x86 TSO but real on weak-memory hardware).
|
||||
#
|
||||
# ThreadSanitizer is the right validator: it instruments
|
||||
# every memory access on fifo_buffer_t::end and
|
||||
# ::first and would flag a missing-lock regression as a
|
||||
# data race on the producer side, the consumer side, or
|
||||
# the avail check. ASan/UBSan would not catch the lock
|
||||
# removal at all. halt_on_error=1 makes TSan exit
|
||||
# non-zero on the first observed race, which is what we
|
||||
# want for CI: any race here means the production
|
||||
# locking discipline is broken.
|
||||
#
|
||||
# The test mirrors the post-fix locking protocol of
|
||||
# gfx_widgets_msg_queue_push and gfx_widgets_iterate's
|
||||
# FIFO interaction; the widget allocation, font
|
||||
# measurement, and animation push that wrap the actual
|
||||
# FIFO calls in production are not part of the race and
|
||||
# are stripped from the test. If gfx_widgets.c amends
|
||||
# the locking around msg_queue, the verbatim copies in
|
||||
# gfx_widgets_msg_queue_race_test.c must follow.
|
||||
make clean all SANITIZER=thread
|
||||
test -x gfx_widgets_msg_queue_race_test
|
||||
TSAN_OPTIONS=halt_on_error=1 timeout 60 \
|
||||
./gfx_widgets_msg_queue_race_test
|
||||
echo "[pass] gfx_widgets_msg_queue_race_test"
|
||||
+83
-6
@@ -189,6 +189,13 @@ static void msg_widget_msg_transition_animation_done(void *userdata)
|
||||
msg->msg_transition_animation = 0.0f;
|
||||
}
|
||||
|
||||
/* Forward declaration: msg_queue_free is defined further below
|
||||
* (around line 550) but is needed by msg_queue_push for the
|
||||
* race-loss rollback path added in the producer-locking fix. */
|
||||
static void gfx_widgets_msg_queue_free(
|
||||
dispgfx_widget_t *p_dispwidget,
|
||||
disp_widget_msg_t *msg);
|
||||
|
||||
void gfx_widgets_msg_queue_push(
|
||||
retro_task_t *task,
|
||||
const char *msg,
|
||||
@@ -203,7 +210,18 @@ void gfx_widgets_msg_queue_push(
|
||||
disp_widget_msg_t *msg_widget = NULL;
|
||||
dispgfx_widget_t *p_dispwidget = &dispwidget_st;
|
||||
|
||||
if (FIFO_WRITE_AVAIL_NONPTR(p_dispwidget->msg_queue) > 0)
|
||||
/* The outer FIFO_WRITE_AVAIL fast-path check that used to wrap
|
||||
* this function body has been removed: reading the FIFO cursors
|
||||
* outside msg_queue_lock is a data race against the producer
|
||||
* lock-protected fifo_write below (TSan-detectable; benign on
|
||||
* x86 TSO but real on weak-memory hardware). The locked
|
||||
* avail re-check at the fifo_write site is the correctness gate.
|
||||
*
|
||||
* Removing the outer gate also fixes a latent behaviour bug:
|
||||
* the update-existing branch (else clause below) does not write
|
||||
* to the FIFO -- it only mutates an already-tracked widget --
|
||||
* so suppressing it on FIFO-full was wrong. Existing widgets
|
||||
* now get updated regardless of FIFO state. */
|
||||
{
|
||||
/* Get current msg if it exists */
|
||||
if (task && task->frontend_userdata)
|
||||
@@ -376,8 +394,45 @@ void gfx_widgets_msg_queue_push(
|
||||
msg_widget->text_height *= 2;
|
||||
}
|
||||
|
||||
fifo_write(&p_dispwidget->msg_queue,
|
||||
&msg_widget, sizeof(msg_widget));
|
||||
/* Lock the FIFO across the avail re-check + fifo_write so
|
||||
* concurrent producers can't both pass the check and
|
||||
* overwrite each other's data. fifo_write itself does no
|
||||
* bounds checking -- it silently wraps -- so the inner
|
||||
* avail check is the actual gate.
|
||||
*
|
||||
* The outer FIFO_WRITE_AVAIL_NONPTR check at the top of
|
||||
* this function is a fast-path bail and is intentionally
|
||||
* left unlocked: a stale read there at worst causes us to
|
||||
* skip a single message that we could have queued, which
|
||||
* is recoverable on the next call. The check below is the
|
||||
* one whose accuracy matters for correctness. */
|
||||
{
|
||||
#ifdef HAVE_THREADS
|
||||
bool fifo_full;
|
||||
slock_lock(p_dispwidget->msg_queue_lock);
|
||||
fifo_full = (FIFO_WRITE_AVAIL_NONPTR(p_dispwidget->msg_queue)
|
||||
< sizeof(msg_widget));
|
||||
if (!fifo_full)
|
||||
fifo_write(&p_dispwidget->msg_queue,
|
||||
&msg_widget, sizeof(msg_widget));
|
||||
slock_unlock(p_dispwidget->msg_queue_lock);
|
||||
|
||||
if (fifo_full)
|
||||
{
|
||||
/* Lost the race against another producer. Roll back
|
||||
* the widget we just allocated -- nobody else has a
|
||||
* reference to it yet (we only got here from the
|
||||
* spawn-new branch, where msg_widget is freshly
|
||||
* allocated above), so this is safe. */
|
||||
gfx_widgets_msg_queue_free(p_dispwidget, msg_widget);
|
||||
free(msg_widget);
|
||||
return;
|
||||
}
|
||||
#else
|
||||
fifo_write(&p_dispwidget->msg_queue,
|
||||
&msg_widget, sizeof(msg_widget));
|
||||
#endif
|
||||
}
|
||||
}
|
||||
/* Update task info */
|
||||
else
|
||||
@@ -962,9 +1017,16 @@ void gfx_widgets_iterate(
|
||||
|
||||
/* Messages queue */
|
||||
|
||||
/* Consume one message if available */
|
||||
if ((FIFO_READ_AVAIL_NONPTR(p_dispwidget->msg_queue) > 0)
|
||||
&& !(p_dispwidget->flags & DISPGFX_WIDGET_FLAG_MOVING)
|
||||
/* Consume one message if available. The outer condition no
|
||||
* longer reads FIFO_READ_AVAIL_NONPTR -- doing so outside
|
||||
* msg_queue_lock would race with concurrent producer fifo_writes
|
||||
* (TSan-detectable; benign on x86 TSO but real on weak-memory
|
||||
* hardware). The locked re-check at the fifo_read site below
|
||||
* is the correctness gate. current_msgs_size and the MOVING
|
||||
* flag remain in the outer guard -- they are unrelated to the
|
||||
* msg_queue race; their own synchronisation discipline is
|
||||
* handled by current_msgs_lock and is unchanged here. */
|
||||
if ( !(p_dispwidget->flags & DISPGFX_WIDGET_FLAG_MOVING)
|
||||
&& (p_dispwidget->current_msgs_size < ARRAY_SIZE(p_dispwidget->current_msgs)))
|
||||
{
|
||||
disp_widget_msg_t *msg_widget = NULL;
|
||||
@@ -975,9 +1037,21 @@ void gfx_widgets_iterate(
|
||||
|
||||
if (p_dispwidget->current_msgs_size < ARRAY_SIZE(p_dispwidget->current_msgs))
|
||||
{
|
||||
/* Lock around the FIFO read to serialise against
|
||||
* concurrent producer fifo_writes. Held strictly inside
|
||||
* current_msgs_lock (which protects current_msgs[]) --
|
||||
* lock order is consistent with all other call sites:
|
||||
* msg_queue_lock is always the inner lock when both
|
||||
* are held. */
|
||||
#ifdef HAVE_THREADS
|
||||
slock_lock(p_dispwidget->msg_queue_lock);
|
||||
#endif
|
||||
if (FIFO_READ_AVAIL_NONPTR(p_dispwidget->msg_queue) > 0)
|
||||
fifo_read(&p_dispwidget->msg_queue,
|
||||
&msg_widget, sizeof(msg_widget));
|
||||
#ifdef HAVE_THREADS
|
||||
slock_unlock(p_dispwidget->msg_queue_lock);
|
||||
#endif
|
||||
|
||||
if (msg_widget)
|
||||
{
|
||||
@@ -1999,6 +2073,8 @@ static void gfx_widgets_free(dispgfx_widget_t *p_dispwidget)
|
||||
|
||||
slock_free(p_dispwidget->current_msgs_lock);
|
||||
p_dispwidget->current_msgs_lock = NULL;
|
||||
slock_free(p_dispwidget->msg_queue_lock);
|
||||
p_dispwidget->msg_queue_lock = NULL;
|
||||
#endif
|
||||
|
||||
p_dispwidget->msg_queue_tasks_count = 0;
|
||||
@@ -2150,6 +2226,7 @@ bool gfx_widgets_init(
|
||||
|
||||
#ifdef HAVE_THREADS
|
||||
p_dispwidget->current_msgs_lock = slock_new();
|
||||
p_dispwidget->msg_queue_lock = slock_new();
|
||||
#endif
|
||||
|
||||
fill_pathname_join_special(
|
||||
|
||||
@@ -191,6 +191,22 @@ typedef struct dispgfx_widget
|
||||
|
||||
#ifdef HAVE_THREADS
|
||||
slock_t* current_msgs_lock;
|
||||
/* Serialises producer and consumer access to msg_queue.
|
||||
* Producers (gfx_widgets_msg_queue_push) can be called from
|
||||
* any thread -- the threaded task system at libretro-common/
|
||||
* queues/task_queue.c runs a worker thread, and several call
|
||||
* paths reach the producer without holding any other lock
|
||||
* (notably gfx/video_driver.c::video_driver_frame, which
|
||||
* releases RUNLOOP_MSG_QUEUE_LOCK before the call). The
|
||||
* consumer (gfx_widgets_iterate) runs on the main thread
|
||||
* but did not previously serialise its fifo_read against
|
||||
* concurrent producer fifo_writes. This separates concerns:
|
||||
* current_msgs_lock continues to guard the displayed-message
|
||||
* deque (current_msgs[]); msg_queue_lock guards the FIFO
|
||||
* staging buffer. Acquired by every fifo_* call site and
|
||||
* by FIFO_*_AVAIL checks where the result is used to gate a
|
||||
* subsequent FIFO operation. */
|
||||
slock_t* msg_queue_lock;
|
||||
#endif
|
||||
fifo_buffer_t msg_queue;
|
||||
disp_widget_msg_t* current_msgs[MSG_QUEUE_ONSCREEN_MAX];
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
TARGET := gfx_widgets_msg_queue_race_test
|
||||
|
||||
# Path back to the repo root from this sample dir. The test references
|
||||
# headers from libretro-common/include and links against
|
||||
# libretro-common/queues/fifo_queue.c (the FIFO this regression test
|
||||
# exercises) and libretro-common/rthreads/rthreads.c (when HAVE_THREADS
|
||||
# is enabled).
|
||||
REPO_ROOT := ../../..
|
||||
LIBRETRO_COMM_DIR := $(REPO_ROOT)/libretro-common
|
||||
|
||||
# This sample mirrors the post-fix locking discipline around
|
||||
# dispgfx_widget_t::msg_queue in gfx/gfx_widgets.c. The test uses
|
||||
# fifo_buffer_t directly (compiled from libretro-common) but does NOT
|
||||
# pull in gfx_widgets.c itself -- the fix is a locking protocol,
|
||||
# not a code change to the FIFO primitive.
|
||||
HAVE_THREADS ?= 1
|
||||
|
||||
SOURCES := \
|
||||
gfx_widgets_msg_queue_race_test.c \
|
||||
$(LIBRETRO_COMM_DIR)/queues/fifo_queue.c
|
||||
|
||||
CFLAGS += -Wall -pedantic -std=gnu99 -g -O0 \
|
||||
-I$(LIBRETRO_COMM_DIR)/include
|
||||
|
||||
ifeq ($(HAVE_THREADS),1)
|
||||
CFLAGS += -DHAVE_THREADS
|
||||
SOURCES += $(LIBRETRO_COMM_DIR)/rthreads/rthreads.c
|
||||
LDFLAGS += -lpthread
|
||||
# rthreads.c uses clock_gettime + CLOCK_REALTIME on Linux glibc; on
|
||||
# older glibc those live in -lrt. Harmless on newer glibc.
|
||||
LDFLAGS += -lrt
|
||||
endif
|
||||
|
||||
OBJS := $(SOURCES:.c=.o)
|
||||
|
||||
ifneq ($(SANITIZER),)
|
||||
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
|
||||
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
|
||||
endif
|
||||
|
||||
all: $(TARGET)
|
||||
|
||||
%.o: %.c
|
||||
$(CC) -c -o $@ $< $(CFLAGS)
|
||||
|
||||
$(TARGET): $(OBJS)
|
||||
$(CC) -o $@ $^ $(LDFLAGS)
|
||||
|
||||
clean:
|
||||
rm -f $(TARGET) $(OBJS)
|
||||
|
||||
.PHONY: clean
|
||||
@@ -0,0 +1,497 @@
|
||||
/* Copyright (C) 2010-2026 The RetroArch team
|
||||
*
|
||||
* ---------------------------------------------------------------------------------------
|
||||
* The following license statement only applies to this file (gfx_widgets_msg_queue_race_test.c).
|
||||
* ---------------------------------------------------------------------------------------
|
||||
*
|
||||
* Permission is hereby granted, free of charge,
|
||||
* to any person obtaining a copy of this software and associated documentation files (the "Software"),
|
||||
* to deal in the Software without restriction, including without limitation the rights to
|
||||
* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software,
|
||||
* and to permit persons to whom the Software is furnished to do so, subject to the following conditions:
|
||||
*
|
||||
* The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
|
||||
*
|
||||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
|
||||
* INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
|
||||
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
|
||||
* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
|
||||
* WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
|
||||
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
|
||||
*/
|
||||
|
||||
/* Regression test for the producer-producer race fix on
|
||||
* dispgfx_widget_t::msg_queue in gfx/gfx_widgets.c.
|
||||
*
|
||||
* Pre-fix: gfx_widgets_msg_queue_push called fifo_write on
|
||||
* p_dispwidget->msg_queue without holding any lock. The
|
||||
* function is reachable from three callers:
|
||||
*
|
||||
* 1. runloop.c:runloop_msg_queue_push (40+ files transitively
|
||||
* reach this; many run on threaded task workers via
|
||||
* libretro-common/queues/task_queue.c). Holds
|
||||
* RUNLOOP_MSG_QUEUE_LOCK across the call.
|
||||
* 2. runloop.c:runloop_task_msg_queue_push. Same lock.
|
||||
* 3. gfx/video_driver.c:video_driver_frame. Acquires
|
||||
* RUNLOOP_MSG_QUEUE_LOCK to drain runloop_st->msg_queue,
|
||||
* RELEASES it, then calls gfx_widgets_msg_queue_push
|
||||
* WITHOUT the lock.
|
||||
*
|
||||
* Path 3 runs on the main thread; path 1 can run on any thread.
|
||||
* RUNLOOP_MSG_QUEUE_LOCK protects runloop_st->msg_queue, NOT
|
||||
* p_dispwidget->msg_queue -- the lock name reflects the runloop
|
||||
* struct it was named after. So path 3's fifo_write (no lock)
|
||||
* can race with path 1's fifo_write (lock held but irrelevant
|
||||
* to path 3).
|
||||
*
|
||||
* The consumer (gfx_widgets_iterate -> fifo_read) ran inside
|
||||
* RUNLOOP_MSG_QUEUE_LOCK at runloop.c:6048 and inside
|
||||
* current_msgs_lock at gfx_widgets.c:973 -- but neither lock
|
||||
* is taken on path 3, so the consumer's fifo_read could observe
|
||||
* a half-updated `end` cursor from path 3's fifo_write.
|
||||
*
|
||||
* Outcome of a race: torn `end` cursor publishes a bad pointer
|
||||
* in the disp_widget_msg_t* slots. fifo_read returns garbage,
|
||||
* the consumer dereferences it as a disp_widget_msg_t* in
|
||||
* gfx_widgets_iterate, and we get a use-after-free / segfault.
|
||||
* On x86 TSO the bug is masked most of the time by hardware
|
||||
* ordering; on Win-on-ARM / Apple Silicon (AArch64) it surfaces
|
||||
* more often.
|
||||
*
|
||||
* Fix: a dedicated msg_queue_lock on dispgfx_widget_t, acquired
|
||||
* inside gfx_widgets_msg_queue_push around the fifo_write
|
||||
* (with an avail re-check, since fifo_write does no bounds
|
||||
* checking) and inside gfx_widgets_iterate around the fifo_read.
|
||||
* msg_queue_lock is the inner lock; current_msgs_lock is the
|
||||
* outer lock when both are held.
|
||||
*
|
||||
* This test exercises the post-fix shape:
|
||||
*
|
||||
* - Two producer threads each call a stripped-down version of
|
||||
* gfx_widgets_msg_queue_push that mirrors the post-fix
|
||||
* locking discipline (msg_queue_lock around fifo_write +
|
||||
* avail re-check + rollback on race-loss).
|
||||
*
|
||||
* - One consumer thread calls a stripped-down version of the
|
||||
* gfx_widgets_iterate FIFO-handling block: take
|
||||
* msg_queue_lock, fifo_read, release. Then validate the
|
||||
* pointer it got is one we actually pushed.
|
||||
*
|
||||
* - Run for STRESS_ITERS iterations under ThreadSanitizer.
|
||||
* halt_on_error=1 means the first race aborts the run
|
||||
* non-zero, which the workflow gates on. If the locking
|
||||
* in the production code is removed or weakened (e.g.
|
||||
* someone drops the lock around fifo_write thinking the
|
||||
* outer FIFO_WRITE_AVAIL check is enough), TSan will see
|
||||
* concurrent writes to fifo_buffer_t::end and flag the race.
|
||||
*
|
||||
* The test does NOT mirror the full gfx_widgets_msg_queue_push
|
||||
* function -- the message-widget allocation, font measurement,
|
||||
* and animation push are irrelevant to the race. What matters
|
||||
* is the lock protocol around the fifo_buffer_t access, which
|
||||
* is what we exercise.
|
||||
*
|
||||
* If gfx_widgets.c amends the locking around msg_queue, the
|
||||
* verbatim copies in this test must follow. Convention used by
|
||||
* gfx_thumbnail_status_atomic_test, vulkan_extension_count_test,
|
||||
* and others under samples/.
|
||||
*/
|
||||
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <stdint.h>
|
||||
#include <string.h>
|
||||
|
||||
#include <queues/fifo_queue.h>
|
||||
#include <retro_atomic.h>
|
||||
|
||||
#ifdef HAVE_THREADS
|
||||
#include <rthreads/rthreads.h>
|
||||
#endif
|
||||
|
||||
/* MSG_QUEUE_PENDING_MAX in gfx_widgets.c. Mirrored here for
|
||||
* realism, but the test works at any reasonable size. */
|
||||
#define MSG_QUEUE_PENDING_MAX 32
|
||||
|
||||
/* Stand-in for disp_widget_msg_t. The real struct has 30+
|
||||
* fields; the FIFO only stores the POINTER, not the contents.
|
||||
* For race-checking we just need allocations whose addresses
|
||||
* we can validate. */
|
||||
typedef struct test_msg
|
||||
{
|
||||
uint32_t generation;
|
||||
uint32_t producer_id;
|
||||
} test_msg_t;
|
||||
|
||||
/* Mirror of dispgfx_widget_t's msg_queue + lock pieces. The
|
||||
* real struct has many more fields; the test only models the
|
||||
* synchronisation invariant. */
|
||||
typedef struct
|
||||
{
|
||||
fifo_buffer_t msg_queue;
|
||||
#ifdef HAVE_THREADS
|
||||
slock_t *msg_queue_lock;
|
||||
#endif
|
||||
} dispgfx_widget_test_t;
|
||||
|
||||
/* === Verbatim mirror of gfx_widgets_msg_queue_push's FIFO
|
||||
* interaction (post-fix). Strips out the widget-allocation,
|
||||
* font-measurement, and animation-push work that is not
|
||||
* part of the race; what remains is the lock protocol.
|
||||
* If gfx_widgets.c amends the locking, this block must
|
||||
* follow. === */
|
||||
static int test_msg_queue_push(
|
||||
dispgfx_widget_test_t *p_dispwidget,
|
||||
test_msg_t *msg_widget)
|
||||
{
|
||||
/* No outer FIFO_WRITE_AVAIL fast-path: reading the FIFO
|
||||
* cursors outside msg_queue_lock would race with concurrent
|
||||
* producer fifo_writes (TSan-detectable). The locked
|
||||
* avail re-check below is the correctness gate. */
|
||||
#ifdef HAVE_THREADS
|
||||
bool fifo_full;
|
||||
slock_lock(p_dispwidget->msg_queue_lock);
|
||||
fifo_full = (FIFO_WRITE_AVAIL_NONPTR(p_dispwidget->msg_queue)
|
||||
< sizeof(msg_widget));
|
||||
if (!fifo_full)
|
||||
fifo_write(&p_dispwidget->msg_queue,
|
||||
&msg_widget, sizeof(msg_widget));
|
||||
slock_unlock(p_dispwidget->msg_queue_lock);
|
||||
|
||||
if (fifo_full)
|
||||
return 0; /* race-loss or full: caller's responsibility to free */
|
||||
return 1;
|
||||
#else
|
||||
if (FIFO_WRITE_AVAIL_NONPTR(p_dispwidget->msg_queue) < sizeof(msg_widget))
|
||||
return 0;
|
||||
fifo_write(&p_dispwidget->msg_queue,
|
||||
&msg_widget, sizeof(msg_widget));
|
||||
return 1;
|
||||
#endif
|
||||
}
|
||||
|
||||
/* === Verbatim mirror of gfx_widgets_iterate's FIFO read
|
||||
* block (post-fix). Strips out the current_msgs[] insertion
|
||||
* logic; what remains is the lock protocol. If gfx_widgets.c
|
||||
* amends the locking, this block must follow. === */
|
||||
static test_msg_t *test_msg_queue_consume(
|
||||
dispgfx_widget_test_t *p_dispwidget)
|
||||
{
|
||||
test_msg_t *msg_widget = NULL;
|
||||
|
||||
/* No outer FIFO_READ_AVAIL fast-path: reading the FIFO cursors
|
||||
* outside msg_queue_lock would race with concurrent producer
|
||||
* fifo_writes. The locked re-check below is the gate. */
|
||||
#ifdef HAVE_THREADS
|
||||
slock_lock(p_dispwidget->msg_queue_lock);
|
||||
#endif
|
||||
if (FIFO_READ_AVAIL_NONPTR(p_dispwidget->msg_queue) >= sizeof(msg_widget))
|
||||
fifo_read(&p_dispwidget->msg_queue,
|
||||
&msg_widget, sizeof(msg_widget));
|
||||
#ifdef HAVE_THREADS
|
||||
slock_unlock(p_dispwidget->msg_queue_lock);
|
||||
#endif
|
||||
return msg_widget;
|
||||
}
|
||||
/* === end verbatim copies === */
|
||||
|
||||
#ifdef HAVE_THREADS
|
||||
|
||||
/* Number of pushes per producer thread. 500K * 2 producers gives
|
||||
* 1M total push attempts. Enough to flush out a missing-lock
|
||||
* regression under TSan within a few seconds. TSan halt_on_error=1
|
||||
* means the first observed race aborts the run, so the iteration
|
||||
* count primarily affects how reliably we trigger the bug if it
|
||||
* regresses, not the cost of the success case. */
|
||||
#define STRESS_ITERS 500000
|
||||
|
||||
typedef struct
|
||||
{
|
||||
dispgfx_widget_test_t *widget;
|
||||
uint32_t producer_id;
|
||||
uint32_t pushed_ok;
|
||||
uint32_t pushed_full;
|
||||
} producer_ctx_t;
|
||||
|
||||
typedef struct
|
||||
{
|
||||
dispgfx_widget_test_t *widget;
|
||||
uint32_t consumed;
|
||||
int failed; /* set non-zero on a bogus pointer */
|
||||
retro_atomic_int_t *stop;
|
||||
} consumer_ctx_t;
|
||||
|
||||
static void producer_thread(void *arg)
|
||||
{
|
||||
producer_ctx_t *ctx = (producer_ctx_t*)arg;
|
||||
uint32_t i;
|
||||
for (i = 0; i < STRESS_ITERS; i++)
|
||||
{
|
||||
test_msg_t *m = (test_msg_t*)malloc(sizeof(*m));
|
||||
if (!m)
|
||||
continue; /* OOM: skip; cosmetic-error path */
|
||||
m->generation = i;
|
||||
m->producer_id = ctx->producer_id;
|
||||
|
||||
if (test_msg_queue_push(ctx->widget, m))
|
||||
ctx->pushed_ok++;
|
||||
else
|
||||
{
|
||||
/* Lost the race or FIFO full. Free our allocation. */
|
||||
free(m);
|
||||
ctx->pushed_full++;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
static void consumer_thread(void *arg)
|
||||
{
|
||||
consumer_ctx_t *ctx = (consumer_ctx_t*)arg;
|
||||
while (!retro_atomic_load_acquire_int(ctx->stop))
|
||||
{
|
||||
test_msg_t *m = test_msg_queue_consume(ctx->widget);
|
||||
if (m)
|
||||
{
|
||||
/* Validate that what we got back is plausibly one of
|
||||
* the things a producer pushed. generation < STRESS_ITERS,
|
||||
* producer_id is one of the small set we spawned. A
|
||||
* torn pointer from a missing-lock race would typically
|
||||
* fail this check (garbage memory), but the primary
|
||||
* regression signal here is TSan flagging the
|
||||
* concurrent unsynchronised access on the FIFO cursors,
|
||||
* not this content check. This is belt-and-suspenders. */
|
||||
if (m->generation >= STRESS_ITERS || m->producer_id >= 16)
|
||||
{
|
||||
ctx->failed = 1;
|
||||
free(m);
|
||||
return;
|
||||
}
|
||||
ctx->consumed++;
|
||||
free(m);
|
||||
}
|
||||
}
|
||||
/* Drain anything left behind after producers stopped. */
|
||||
for (;;)
|
||||
{
|
||||
test_msg_t *m = test_msg_queue_consume(ctx->widget);
|
||||
if (!m)
|
||||
break;
|
||||
ctx->consumed++;
|
||||
free(m);
|
||||
}
|
||||
}
|
||||
|
||||
static int run_stress(void)
|
||||
{
|
||||
dispgfx_widget_test_t widget;
|
||||
producer_ctx_t prod_ctx[2];
|
||||
consumer_ctx_t cons_ctx;
|
||||
sthread_t *prod_threads[2];
|
||||
sthread_t *cons_thread;
|
||||
retro_atomic_int_t stop;
|
||||
int i;
|
||||
uint32_t total_pushed = 0;
|
||||
|
||||
retro_atomic_int_init(&stop, 0);
|
||||
|
||||
if (!fifo_initialize(&widget.msg_queue,
|
||||
MSG_QUEUE_PENDING_MAX * sizeof(test_msg_t*)))
|
||||
{
|
||||
fprintf(stderr, "fifo_initialize failed\n");
|
||||
return 1;
|
||||
}
|
||||
widget.msg_queue_lock = slock_new();
|
||||
if (!widget.msg_queue_lock)
|
||||
{
|
||||
fprintf(stderr, "slock_new failed\n");
|
||||
fifo_deinitialize(&widget.msg_queue);
|
||||
return 1;
|
||||
}
|
||||
|
||||
for (i = 0; i < 2; i++)
|
||||
{
|
||||
prod_ctx[i].widget = &widget;
|
||||
prod_ctx[i].producer_id = (uint32_t)i;
|
||||
prod_ctx[i].pushed_ok = 0;
|
||||
prod_ctx[i].pushed_full = 0;
|
||||
}
|
||||
cons_ctx.widget = &widget;
|
||||
cons_ctx.consumed = 0;
|
||||
cons_ctx.failed = 0;
|
||||
cons_ctx.stop = &stop;
|
||||
|
||||
cons_thread = sthread_create(consumer_thread, &cons_ctx);
|
||||
prod_threads[0] = sthread_create(producer_thread, &prod_ctx[0]);
|
||||
prod_threads[1] = sthread_create(producer_thread, &prod_ctx[1]);
|
||||
|
||||
if (!cons_thread || !prod_threads[0] || !prod_threads[1])
|
||||
{
|
||||
fprintf(stderr, "sthread_create failed\n");
|
||||
return 1;
|
||||
}
|
||||
|
||||
sthread_join(prod_threads[0]);
|
||||
sthread_join(prod_threads[1]);
|
||||
retro_atomic_store_release_int(&stop, 1);
|
||||
sthread_join(cons_thread);
|
||||
|
||||
/* Drain anything still in the FIFO (consumer-side, single-
|
||||
* threaded by this point so no lock needed -- but using the
|
||||
* same helper for consistency). */
|
||||
for (;;)
|
||||
{
|
||||
test_msg_t *m = test_msg_queue_consume(&widget);
|
||||
if (!m)
|
||||
break;
|
||||
cons_ctx.consumed++;
|
||||
free(m);
|
||||
}
|
||||
|
||||
total_pushed = prod_ctx[0].pushed_ok + prod_ctx[1].pushed_ok;
|
||||
|
||||
if (cons_ctx.failed)
|
||||
{
|
||||
fprintf(stderr, "FAIL: consumer saw torn / bogus pointer\n");
|
||||
slock_free(widget.msg_queue_lock);
|
||||
fifo_deinitialize(&widget.msg_queue);
|
||||
return 1;
|
||||
}
|
||||
|
||||
if (cons_ctx.consumed != total_pushed)
|
||||
{
|
||||
fprintf(stderr,
|
||||
"FAIL: consumed %u != pushed %u (P0=%u/%u, P1=%u/%u)\n",
|
||||
cons_ctx.consumed, total_pushed,
|
||||
prod_ctx[0].pushed_ok, prod_ctx[0].pushed_full,
|
||||
prod_ctx[1].pushed_ok, prod_ctx[1].pushed_full);
|
||||
slock_free(widget.msg_queue_lock);
|
||||
fifo_deinitialize(&widget.msg_queue);
|
||||
return 1;
|
||||
}
|
||||
|
||||
printf("[pass] stress: %u pushed (%u+%u), %u rejected on full, "
|
||||
"%u consumed, no torn pointers\n",
|
||||
total_pushed, prod_ctx[0].pushed_ok, prod_ctx[1].pushed_ok,
|
||||
prod_ctx[0].pushed_full + prod_ctx[1].pushed_full,
|
||||
cons_ctx.consumed);
|
||||
|
||||
slock_free(widget.msg_queue_lock);
|
||||
fifo_deinitialize(&widget.msg_queue);
|
||||
return 0;
|
||||
}
|
||||
|
||||
#endif /* HAVE_THREADS */
|
||||
|
||||
/* Property-style smoke test for the single-threaded path:
|
||||
* push fills the FIFO, push past full returns 0, consume drains
|
||||
* everything in order. */
|
||||
static int run_smoke(void)
|
||||
{
|
||||
dispgfx_widget_test_t widget;
|
||||
int i;
|
||||
int pushed = 0;
|
||||
int consumed = 0;
|
||||
test_msg_t *messages[MSG_QUEUE_PENDING_MAX + 4];
|
||||
|
||||
if (!fifo_initialize(&widget.msg_queue,
|
||||
MSG_QUEUE_PENDING_MAX * sizeof(test_msg_t*)))
|
||||
{
|
||||
fprintf(stderr, "fifo_initialize failed\n");
|
||||
return 1;
|
||||
}
|
||||
#ifdef HAVE_THREADS
|
||||
widget.msg_queue_lock = slock_new();
|
||||
if (!widget.msg_queue_lock)
|
||||
{
|
||||
fprintf(stderr, "slock_new failed\n");
|
||||
fifo_deinitialize(&widget.msg_queue);
|
||||
return 1;
|
||||
}
|
||||
#endif
|
||||
|
||||
/* Push more than the FIFO can hold; later pushes should
|
||||
* return 0 (rejected on full). */
|
||||
for (i = 0; i < MSG_QUEUE_PENDING_MAX + 4; i++)
|
||||
{
|
||||
messages[i] = (test_msg_t*)malloc(sizeof(test_msg_t));
|
||||
if (!messages[i])
|
||||
{
|
||||
fprintf(stderr, "malloc failed\n");
|
||||
return 1;
|
||||
}
|
||||
messages[i]->generation = i;
|
||||
messages[i]->producer_id = 0;
|
||||
if (test_msg_queue_push(&widget, messages[i]))
|
||||
pushed++;
|
||||
else
|
||||
{
|
||||
/* push refused: free the allocation we tried to enqueue */
|
||||
free(messages[i]);
|
||||
messages[i] = NULL;
|
||||
}
|
||||
}
|
||||
|
||||
if (pushed == 0 || pushed > MSG_QUEUE_PENDING_MAX)
|
||||
{
|
||||
fprintf(stderr, "FAIL: smoke pushed=%d; expected 1..%d\n",
|
||||
pushed, MSG_QUEUE_PENDING_MAX);
|
||||
return 1;
|
||||
}
|
||||
|
||||
/* Drain. Order should be FIFO. */
|
||||
for (i = 0; i < pushed; i++)
|
||||
{
|
||||
test_msg_t *m = test_msg_queue_consume(&widget);
|
||||
if (!m)
|
||||
{
|
||||
fprintf(stderr, "FAIL: smoke consume returned NULL at i=%d\n", i);
|
||||
return 1;
|
||||
}
|
||||
if (m->generation != (uint32_t)i)
|
||||
{
|
||||
fprintf(stderr,
|
||||
"FAIL: smoke order: expected generation %d, got %u\n",
|
||||
i, m->generation);
|
||||
free(m);
|
||||
return 1;
|
||||
}
|
||||
free(m);
|
||||
consumed++;
|
||||
}
|
||||
|
||||
if (test_msg_queue_consume(&widget))
|
||||
{
|
||||
fprintf(stderr, "FAIL: smoke FIFO non-empty after drain\n");
|
||||
return 1;
|
||||
}
|
||||
|
||||
/* Free any allocations whose push was refused. */
|
||||
for (i = pushed; i < MSG_QUEUE_PENDING_MAX + 4; i++)
|
||||
{
|
||||
if (messages[i])
|
||||
free(messages[i]);
|
||||
}
|
||||
|
||||
printf("[pass] smoke: pushed=%d consumed=%d (FIFO order verified)\n",
|
||||
pushed, consumed);
|
||||
|
||||
#ifdef HAVE_THREADS
|
||||
slock_free(widget.msg_queue_lock);
|
||||
#endif
|
||||
fifo_deinitialize(&widget.msg_queue);
|
||||
return 0;
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
if (run_smoke())
|
||||
return 1;
|
||||
#ifdef HAVE_THREADS
|
||||
if (run_stress())
|
||||
return 1;
|
||||
#else
|
||||
printf("[skip] stress: HAVE_THREADS not enabled\n");
|
||||
#endif
|
||||
printf("ALL OK\n");
|
||||
return 0;
|
||||
}
|
||||
Reference in new issue
Block a user