Skip to content

Commit 1a13965

Browse files
committed
libretro-common/queues: bound fifo_write/read + reject SIZE_MAX init
Audit-pass on libretro-common/queues/fifo_queue.c found three defensive gaps. Each is currently latent (audio + gfx widget callers all gate on FIFO_WRITE_AVAIL / FIFO_READ_AVAIL before calling fifo_write / fifo_read) but the function trusts @len unconditionally, so any future caller that forgets the gate silently corrupts memory rather than dropping the request. * fifo_write: out-of-bounds write when @len exceeds available space (libretro-common/queues/fifo_queue.c). The wrap-around branch computes rest_write = len - first_write and memcpy()s rest_write bytes starting at buffer->buffer; for any @len exceeding @buffer->size, that second memcpy walks off the end of the backing allocation. Worse, the gating test `buffer->end + len > buffer->size` itself wraps in size_t for huge @len, mis-routing the call into a single-memcpy branch with @len bytes of unbounded write. Fix by capping @len at FIFO_WRITE_AVAIL(buffer) on entry. No-op for current callers (they already cap); turns the latent overrun into a defined silent truncation for any future caller that forgets to gate. * fifo_read: same out-of-bounds shape on the read side -- rest_read bytes copied past the end of @buffer->buffer when @len exceeds the readable range. Same fix: cap @len at FIFO_READ_AVAIL(buffer) on entry. * fifo_initialize_internal: SIZE_MAX wrap (libretro-common/ queues/fifo_queue.c). The function reserves one ring slot for the empty/full distinction, so the actual allocation is (len + 1) bytes. Passing SIZE_MAX wraps that addition to 0; calloc(1, 0) is allowed to satisfy with a non-NULL pointer to a zero-byte allocation, so initialisation can succeed and leave buf->size == 0. The next fifo_write would then divide by zero on the `% buffer->size` end-pointer update. No current caller asks for SIZE_MAX, so the rejection is purely defensive. Also documents the new caps in the public header (fifo_write / fifo_read) so callers know that exceeding available space is a silent drop, not a silent overrun. Adds a regression test under libretro-common/samples/queues/ fifo_queue_bounds_test/. The test exercises: - SIZE_MAX initialisation (must be rejected), - over-write of 2048 bytes into a 100-byte ring (capped at 100, no OOB write), - over-read into a 64-byte buffer (capped at FIFO_READ_AVAIL, trailing bytes untouched), - SIZE_MAX @len passed to fifo_write with end != 0 (the integer-overflow case in the original gating test), - wrap-around correctness when the cap isn't engaged, - zero-length write/read no-ops. Wired into the existing libretro-common-samples CI workflow with SANITIZER=address,undefined. Pre-patch the test fails: the SIZE_MAX init succeeds (no rejection), and the over-write trips ASan with a 1947-byte heap-buffer-overflow WRITE. Post-patch all seven cases pass under ASan + UBSan + LSan. Note on residual scope ---------------------- The cap protects @buffer->buffer (the destination ring) but not @in_buf (the caller-supplied source). fifo_write reads exactly @len bytes from @in_buf -- a buggy caller that passes @len > sizeof(*in_buf) gets a source over-read that we cannot detect from inside the function (no @in_buf size parameter). That residual is unchanged by this commit; the surface area for it is much smaller than the destination overrun (audio drivers all pass full-size frame buffers). Any future hardening would require an API break to add @in_buf_len, which is out of scope.
1 parent 3281572 commit 1a13965

5 files changed

Lines changed: 374 additions & 3 deletions

File tree

.github/workflows/Linux-libretro-common-samples.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ jobs:
7979
word_wrap_overflow_test
8080
task_queue_title_error_test
8181
tpool_wait_test
82+
fifo_queue_bounds_test
8283
)
8384
8485
# Per-binary run command (overrides ./<binary> if present).

libretro-common/include/queues/fifo_queue.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,12 @@ static INLINE void fifo_clear(fifo_buffer_t *buffer)
126126
/**
127127
* Writes \c size bytes to the given queue.
128128
*
129+
* \c size is silently capped at \c FIFO_WRITE_AVAIL(buffer) --
130+
* the call writes at most that many bytes and discards any
131+
* excess. Callers that need to be sure all bytes are queued
132+
* must gate on \c FIFO_WRITE_AVAIL beforehand. Behaviour is
133+
* undefined if \c buffer is \c NULL.
134+
*
129135
* @param buffer The FIFO queue to write to.
130136
* @param in_buf The buffer to read bytes from.
131137
* @param size The length of \c in_buf, in bytes.
@@ -135,6 +141,12 @@ void fifo_write(fifo_buffer_t *buffer, const void *in_buf, size_t len);
135141
/**
136142
* Reads \c size bytes from the given queue.
137143
*
144+
* \c size is silently capped at \c FIFO_READ_AVAIL(buffer) --
145+
* the call returns at most that many bytes and leaves the
146+
* trailing portion of \c in_buf untouched. Callers that need
147+
* exactly \c size bytes must gate on \c FIFO_READ_AVAIL
148+
* beforehand. Behaviour is undefined if \c buffer is \c NULL.
149+
*
138150
* @param buffer The FIFO queue to read from.
139151
* @param in_buf The buffer to store the read bytes in.
140152
* @param size The length of \c in_buf, in bytes.

libretro-common/queues/fifo_queue.c

Lines changed: 54 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,21 @@
3131

3232
static bool fifo_initialize_internal(fifo_buffer_t *buf, size_t len)
3333
{
34-
uint8_t *buffer = (uint8_t*)calloc(1, len + 1);
34+
uint8_t *buffer;
35+
36+
/* The ring reserves one slot to distinguish empty from full,
37+
* so the actual allocation is (len + 1) bytes. Reject @len
38+
* values that would wrap that addition: SIZE_MAX would
39+
* compute (size_t)0, which calloc(1, 0) is allowed to satisfy
40+
* with a non-NULL pointer to a zero-byte allocation. Letting
41+
* that succeed would leave buf->size == 0 and the next
42+
* fifo_write would divide by zero at the `% buffer->size`
43+
* step. No current caller asks for SIZE_MAX, so the rejection
44+
* is purely defensive. */
45+
if (len >= SIZE_MAX)
46+
return false;
47+
48+
buffer = (uint8_t*)calloc(1, len + 1);
3549

3650
if (!buffer)
3751
return false;
@@ -91,8 +105,31 @@ fifo_buffer_t *fifo_new(size_t len)
91105

92106
void fifo_write(fifo_buffer_t *buffer, const void *in_buf, size_t len)
93107
{
94-
size_t first_write = len;
108+
size_t first_write;
95109
size_t rest_write = 0;
110+
size_t avail;
111+
112+
/* Cap @len at the available space. Existing callers all
113+
* gate on FIFO_WRITE_AVAIL before invoking us, so this is
114+
* a no-op for them; for any caller that doesn't, the
115+
* unbounded branch below would walk off the end of
116+
* @buffer->buffer (the wrap-around copy at line `memcpy(
117+
* buffer->buffer, ..., rest_write)` would write up to
118+
* len - first_write bytes into a buffer of @buffer->size
119+
* total, overrunning by len - size). Worse, the original
120+
* `buffer->end + len > buffer->size` test wraps in size_t
121+
* for huge @len and silently misclassifies the request as
122+
* "fits in one chunk", taking the corrupting first memcpy
123+
* down a path with no wrap-around bound at all. Capping
124+
* here closes both windows. */
125+
avail = FIFO_WRITE_AVAIL(buffer);
126+
if (len > avail)
127+
len = avail;
128+
129+
if (!len)
130+
return;
131+
132+
first_write = len;
96133

97134
if (buffer->end + len > buffer->size)
98135
{
@@ -109,8 +146,22 @@ void fifo_write(fifo_buffer_t *buffer, const void *in_buf, size_t len)
109146

110147
void fifo_read(fifo_buffer_t *buffer, void *in_buf, size_t len)
111148
{
112-
size_t first_read = len;
149+
size_t first_read;
113150
size_t rest_read = 0;
151+
size_t avail;
152+
153+
/* Same rationale as fifo_write: cap @len at what's actually
154+
* available to avoid out-of-buffer copies on a caller that
155+
* forgot to gate on FIFO_READ_AVAIL. Existing callers all
156+
* gate first; this is defensive. */
157+
avail = FIFO_READ_AVAIL(buffer);
158+
if (len > avail)
159+
len = avail;
160+
161+
if (!len)
162+
return;
163+
164+
first_read = len;
114165

115166
if (buffer->first + len > buffer->size)
116167
{
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
TARGET := fifo_queue_bounds_test
2+
3+
LIBRETRO_COMM_DIR := ../../..
4+
5+
SOURCES := \
6+
fifo_queue_bounds_test.c \
7+
$(LIBRETRO_COMM_DIR)/queues/fifo_queue.c
8+
9+
OBJS := $(SOURCES:.c=.o)
10+
11+
CFLAGS += -Wall -pedantic -std=gnu99 -g -O0 -I$(LIBRETRO_COMM_DIR)/include
12+
13+
ifneq ($(SANITIZER),)
14+
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
15+
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
16+
endif
17+
18+
all: $(TARGET)
19+
20+
%.o: %.c
21+
$(CC) -c -o $@ $< $(CFLAGS)
22+
23+
$(TARGET): $(OBJS)
24+
$(CC) -o $@ $^ $(LDFLAGS)
25+
26+
clean:
27+
rm -f $(TARGET) $(OBJS)
28+
29+
.PHONY: clean

0 commit comments

Comments
 (0)