Skip to content

Commit f5c7df5

Browse files
committed
gfx/common/vulkan: plug partial-init leak in emulated mailbox + CI
Found during the v2 audit of gfx/common/vulkan_common.c, deferred from the v3 patch (da5e650) which addressed the three high-severity heap-corruption findings. Adds an AddressSanitizer + LSan regression test under samples/gfx/vulkan_mailbox_init_leak/ wired into the existing Linux-samples-gfx CI workflow as a fifth step. vulkan_emulated_mailbox_init has three sequential allocations (scond_new, slock_new, sthread_create) and on any of the latter two failures returns `false` directly, leaking the already- allocated cond and/or lock. Both production call sites in vulkan_create_swapchain ignore the return value -- so an init failure also leaves vk->mailbox.lock == NULL and vk->mailbox.cond == NULL while VK_DATA_FLAG_EMULATING_MAILBOX is still set, setting up a NULL-deref the next time vulkan_acquire_next_image() routes into the emulated path (slock_lock(NULL) inside vulkan_emulated_mailbox_acquire_next_image()). The leak itself is small (the libretro-common slock_t / scond_t each hold a pthread mutex / cond worth of data); the trigger requires scond_new or slock_new to fail, which means OOM or pthread resource exhaustion. Realistic mainly on memory- constrained devices (older Android, Vita, 3DS, Wii U) where filter-chain teardown / re-init under thrashing memory pressure can put the system in this state. * gfx/common/vulkan_common.c Route every early-failure path in vulkan_emulated_mailbox_init through `goto error` to a single cleanup that calls vulkan_emulated_mailbox_deinit(). The deinit function is null-safe (each free is guarded by an `if (mailbox->X)` check) and ends with a memset, so the deinit-on-failure shape matches the deinit-on-shutdown shape exactly -- the two production callers that ignore the return value get a self-healing failure mode. The post-deinit memset also clears mailbox->swapchain, which trips the existing guard at vulkan_acquire_next_image: if (vk->mailbox.swapchain == VK_NULL_HANDLE) err = VK_ERROR_OUT_OF_DATE_KHR; closing the NULL-deref crash that the bare `return false` left open. No caller-side changes needed. * samples/gfx/vulkan_mailbox_init_leak/, .github/workflows/Linux-samples-gfx.yml Regression test following the convention of the v3 vulkan/ tests, the v4 slang test, and the security regression tests under samples/tasks/. Keeps verbatim copies of both vulkan_emulated_mailbox_init() and vulkan_emulated_mailbox_ deinit() demarcated by `=== verbatim copy ===` markers, with an `IMPORTANT: ... must follow` comment. Plain C, asserts manually, exits nonzero on failure. Five probes: scond fails first (was already correct, baseline case); slock fails second (pre-fix: scond leak); sthread fails third (pre-fix: scond + slock leak); full success (sanity check on alloc/free pairing post-deinit); and an explicit __lsan_do_recoverable_leak_check() call after running all three failure stages. Mocks scond_new / slock_new / sthread_create with controllable failure injection so each stage can be exercised deterministically. Verified pre/post-patch discrimination: under the pre-fix `return false` shape, ASan's LeakSanitizer reports 384 bytes leaked across 6 allocations (1 scond from slock-fails + 2 alloc/leak pairs from sthread-fails + 3 from the lsan-check probe), each with a full backtrace pointing into vulkan_emulated_mailbox_init. Under the post-fix shape all five probes pass clean. The CI step runs with ASAN_OPTIONS=detect_leaks=1 so leaks fail the run with a non-zero exit.
1 parent de7ae03 commit f5c7df5

4 files changed

Lines changed: 628 additions & 3 deletions

File tree

.github/workflows/Linux-samples-gfx.yml

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,3 +139,36 @@ jobs:
139139
test -x slang_texture_index_bounds_test
140140
timeout 60 ./slang_texture_index_bounds_test
141141
echo "[pass] slang_texture_index_bounds_test"
142+
143+
- name: Build and run vulkan_mailbox_init_leak_test (ASan + LSan)
144+
shell: bash
145+
working-directory: samples/gfx/vulkan_mailbox_init_leak
146+
run: |
147+
set -eu
148+
# Regression test for the partial-init leak fix in
149+
# gfx/common/vulkan_common.c::vulkan_emulated_mailbox_init.
150+
# Pre-fix the function had three sequential allocations
151+
# (scond_new, slock_new, sthread_create) and on any of the
152+
# latter two failures returned `false` directly, leaking
153+
# the already-allocated cond and/or lock. Both production
154+
# call sites in vulkan_create_swapchain ignore the return
155+
# value, so an init failure also left vk->mailbox.lock ==
156+
# NULL and vk->mailbox.cond == NULL while VK_DATA_FLAG_
157+
# EMULATING_MAILBOX was still set, setting up a NULL-deref
158+
# the next time vulkan_acquire_next_image routed into
159+
# vulkan_emulated_mailbox_acquire_next_image (slock_lock
160+
# on a NULL pointer). Fix routes every early failure
161+
# through `goto error` to a single deinit call, which is
162+
# null-safe and ends with a memset, leaving the struct in
163+
# the same shape the deinit-on-shutdown path produces and
164+
# tripping the existing `mailbox.swapchain == VK_NULL_
165+
# HANDLE` guard at vulkan_acquire_next_image. Build under
166+
# AddressSanitizer with leak detection so any
167+
# reintroduction is caught at the leak level. If
168+
# vulkan_common.c amends the init or deinit, the verbatim
169+
# copies in vulkan_mailbox_init_leak_test.c must follow.
170+
make clean all SANITIZER=address
171+
test -x vulkan_mailbox_init_leak_test
172+
ASAN_OPTIONS=detect_leaks=1 timeout 60 \
173+
./vulkan_mailbox_init_leak_test
174+
echo "[pass] vulkan_mailbox_init_leak_test"

gfx/common/vulkan_common.c

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -265,13 +265,26 @@ static bool vulkan_emulated_mailbox_init(
265265
mailbox->flags = 0;
266266

267267
if (!(mailbox->cond = scond_new()))
268-
return false;
268+
goto error;
269269
if (!(mailbox->lock = slock_new()))
270-
return false;
270+
goto error;
271271
if (!(mailbox->thread = sthread_create(vulkan_emulated_mailbox_loop,
272272
mailbox)))
273-
return false;
273+
goto error;
274274
return true;
275+
276+
error:
277+
/* Tear down anything we managed to allocate before failing.
278+
* vulkan_emulated_mailbox_deinit() is null-safe and ends with
279+
* a memset, so the struct is left in the same shape a caller
280+
* would see after a successful init+deinit cycle -- callers
281+
* that ignore our return value (the two sites in
282+
* vulkan_create_swapchain) will then take the
283+
* mailbox.swapchain == VK_NULL_HANDLE branch in
284+
* vulkan_acquire_next_image and skip the emulated path
285+
* cleanly instead of dereferencing a NULL lock/cond. */
286+
vulkan_emulated_mailbox_deinit(mailbox);
287+
return false;
275288
}
276289

277290
static void vulkan_debug_mark_object(VkDevice device,
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
TARGET := vulkan_mailbox_init_leak_test
2+
3+
SOURCES := vulkan_mailbox_init_leak_test.c
4+
5+
OBJS := $(SOURCES:.c=.o)
6+
7+
CFLAGS += -Wall -pedantic -std=gnu99 -g -O0
8+
9+
ifneq ($(SANITIZER),)
10+
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
11+
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
12+
endif
13+
14+
all: $(TARGET)
15+
16+
%.o: %.c
17+
$(CC) -c -o $@ $< $(CFLAGS)
18+
19+
$(TARGET): $(OBJS)
20+
$(CC) -o $@ $^ $(LDFLAGS)
21+
22+
clean:
23+
rm -f $(TARGET) $(OBJS)
24+
25+
.PHONY: clean

0 commit comments

Comments
 (0)