Skip to content

Commit dd3204d

Browse files
committed
gfx/drivers_context/{wayland,w,x}_vk_ctx: fix UAF + double-free + CI
Found during the audit of the Vulkan context shims after f5c7df5. Adds an AddressSanitizer regression test under samples/gfx/ vulkan_ctx_double_free/ wired into the Linux-samples-gfx CI workflow as a sixth step. Three of the five Vulkan context drivers' set_video_mode hooks called their own destroy() (or destroy_resources() + free()) on ctx_data before returning false: wayland_vk_ctx.c:218 gfx_ctx_wl_destroy(data) w_vk_ctx.c:229 gfx_ctx_w_vk_destroy(data) x_vk_ctx.c:489-499 destroy_resources + free The caller in gfx/drivers/vulkan.c::vulkan_init treats a false return from set_video_mode (line 4938) as a failed in-flight vk_t construction and `goto error`s into vulkan_free() (line 5120), which at line 4665 calls vk->ctx_driver->destroy(vk->ctx_data); -- on the very pointer the inner set_video_mode just freed. The ctx_driver->destroy hook then walks the freed struct (gfx_ctx_wl_destroy_resources / gfx_ctx_x_vk_destroy_resources dereference fields off the freed pointer) and free()s the same pointer a second time. On glibc with default malloc the second free is detected (`double free or corruption`) and the process aborts. Under jemalloc / mimalloc / older glibc the second free silently corrupts the heap, with a write-what-where primitive controlled by the allocator's freelist layout. Reachability: vulkan_surface_create() failure (missing extension or driver-level Vulkan error), Wayland's set_video_mode_common_* helpers failing, X11's XGetVisualInfo returning NULL or x11_input_ctx_new failing, win32_set_video_mode failing. None are common but all are reachable on misconfigured systems, headless tests, or CI environments without a working display. * gfx/drivers_context/wayland_vk_ctx.c * gfx/drivers_context/w_vk_ctx.c * gfx/drivers_context/x_vk_ctx.c Drop the destroy/free call from each set_video_mode error path. Cocoa (cocoa_vk_ctx.m) and Android (android_vk_ctx.c) already implement this correctly: just `return false` and let the caller's vulkan_free chain do the single cleanup. This makes the other three match. X11's local XFree(vi) and `g_x11_screen = 0` reset are preserved -- those are local-resource and global-state cleanups respectively, not duplicates of the upper-layer destroy. Wayland and Win32 had no such locals to preserve. Each error-path edit carries an inline comment naming the upper-layer cleanup site (gfx/drivers/vulkan.c::vulkan_free line 4665) so a future maintainer who wonders why the inner destroy is absent doesn't reintroduce it. * samples/gfx/vulkan_ctx_double_free/, .github/workflows/Linux-samples-gfx.yml Regression test following the convention of the v3 vulkan/ tests, the v4 slang test, the v5 mailbox-leak test, and the security regression tests under samples/tasks/. Models the lifecycle contract abstractly rather than building any one driver -- the bug is the shape of the cleanup flow, not a driver-specific quirk. Five probes: post-fix success path (1 alloc / 1 destroy); post-fix failure path (the smoking gun: 1 alloc / 1 destroy, no double-free, no UAF); 16 back-to-back failures (no accumulation, no leaks); interleaved success/failure; and a documentation probe describing how a maintainer can swap the function pointer to re-trigger the pre-fix UAF. Verified pre/post-patch discrimination: under the pre-fix shape ASan reports `double-free` at the upper-layer destroy with the freed allocation traced back to set_video_mode's inner destroy. Under the post-fix shape all five probes pass clean. Wires into Linux-samples-gfx.yml as a sixth step, modeled on the existing five with the same per-step explanation of what the test is regression-testing and the "verbatim copy must follow" reminder.
1 parent dcf3ae3 commit dd3204d

6 files changed

Lines changed: 455 additions & 6 deletions

File tree

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,3 +172,36 @@ jobs:
172172
ASAN_OPTIONS=detect_leaks=1 timeout 60 \
173173
./vulkan_mailbox_init_leak_test
174174
echo "[pass] vulkan_mailbox_init_leak_test"
175+
176+
- name: Build and run vulkan_ctx_double_free_test (ASan)
177+
shell: bash
178+
working-directory: samples/gfx/vulkan_ctx_double_free
179+
run: |
180+
set -eu
181+
# Regression test for the double-free / use-after-free
182+
# fix in the Vulkan context drivers' set_video_mode error
183+
# paths: gfx/drivers_context/wayland_vk_ctx.c,
184+
# w_vk_ctx.c, x_vk_ctx.c. Pre-fix each set_video_mode
185+
# called its own destroy()/destroy_resources()+free() on
186+
# ctx_data before returning false; the caller in
187+
# gfx/drivers/vulkan.c::vulkan_init then ran
188+
# vulkan_free()->ctx_driver->destroy(ctx_data) on the
189+
# already-freed pointer (UAF read of struct fields, then
190+
# a second free of the same allocation). Reachable from
191+
# vulkan_surface_create() failure (missing extension /
192+
# driver issue), Wayland's set_video_mode_common_*
193+
# helpers failing, X11's XGetVisualInfo returning NULL,
194+
# or win32_set_video_mode failing. Cocoa (cocoa_vk_ctx)
195+
# and Android (android_vk_ctx) already handle this
196+
# correctly by returning false without freeing -- the
197+
# fix makes Wayland/Win32/X11 match. Build under
198+
# AddressSanitizer so any reintroduction is caught at
199+
# the bounds level (UAF + double-free both fire). If
200+
# any of the three context drivers amends the
201+
# set_video_mode error path to once again destroy
202+
# ctx_data, the verbatim copy in
203+
# vulkan_ctx_double_free_test.c must follow.
204+
make clean all SANITIZER=address
205+
test -x vulkan_ctx_double_free_test
206+
timeout 60 ./vulkan_ctx_double_free_test
207+
echo "[pass] vulkan_ctx_double_free_test"

gfx/drivers_context/w_vk_ctx.c

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,14 @@ static bool gfx_ctx_w_vk_set_video_mode(void *data,
226226
}
227227

228228
RARCH_ERR("[Vulkan] win32_set_video_mode failed.\n");
229-
gfx_ctx_w_vk_destroy(data);
229+
/* Do not destroy `data` here. The caller in
230+
* gfx/drivers/vulkan.c::vulkan_init treats a false return
231+
* from set_video_mode as a failure of the in-flight `vk_t`
232+
* construction and runs vulkan_free() on it, which calls
233+
* ctx_driver->destroy(ctx_data) -- i.e. gfx_ctx_w_vk_destroy()
234+
* -- on the very pointer we already freed. Leave cleanup
235+
* to the caller's single normal-path destroy. Cocoa /
236+
* Android already do this; this matches them. */
230237
return false;
231238
}
232239

gfx/drivers_context/wayland_vk_ctx.c

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,16 @@ static bool gfx_ctx_wl_set_video_mode(void *data,
216216
return true;
217217

218218
error:
219-
gfx_ctx_wl_destroy(data);
219+
/* Do not destroy `wl` here. The caller in
220+
* gfx/drivers/vulkan.c::vulkan_init treats a false return
221+
* from set_video_mode as a failure of the in-flight `vk_t`
222+
* construction and runs vulkan_free() on it, which calls
223+
* ctx_driver->destroy(ctx_data) -- i.e. gfx_ctx_wl_destroy()
224+
* -- on the very pointer we already freed. That second call
225+
* walks freed memory in gfx_ctx_wl_destroy_resources() and
226+
* then free()s the same pointer again. Leave cleanup to the
227+
* caller's single normal-path destroy. Cocoa / Android
228+
* already do this; this matches them. */
220229
return false;
221230
}
222231

gfx/drivers_context/x_vk_ctx.c

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -490,10 +490,16 @@ static bool gfx_ctx_x_vk_set_video_mode(void *data,
490490
if (vi)
491491
XFree(vi);
492492

493-
gfx_ctx_x_vk_destroy_resources(x);
494-
495-
if (x)
496-
free(x);
493+
/* Do not destroy `x` here. The caller in
494+
* gfx/drivers/vulkan.c::vulkan_init treats a false return
495+
* from set_video_mode as a failure of the in-flight `vk_t`
496+
* construction and runs vulkan_free() on it, which calls
497+
* ctx_driver->destroy(ctx_data) -- i.e. gfx_ctx_x_vk_destroy()
498+
* -- on the very pointer we already freed. That second call
499+
* walks freed memory in gfx_ctx_x_vk_destroy_resources() and
500+
* then free()s the same pointer again. Leave cleanup to the
501+
* caller's single normal-path destroy. Cocoa / Android
502+
* already do this; this matches them. */
497503
g_x11_screen = 0;
498504

499505
return false;
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
TARGET := vulkan_ctx_double_free_test
2+
3+
SOURCES := vulkan_ctx_double_free_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)