Commit 46ba4c1
committed
gfx/common/x11: NULL-guard xss_screensaver_inhibit()
Found by the new ASan+UBSan CI workflow's headless SDL2
imageviewer smoke (b9777c8 + d967813). Run #2 of the smoke
(after the v12 xvideo->sdl2 pivot) caught a SEGV at process
startup:
AddressSanitizer:DEADLYSIGNAL
==9537==ERROR: AddressSanitizer: SEGV on unknown address
0x000000000968. The signal is caused by a READ memory
access.
#0 XQueryExtension (libX11.so.6+0x361f6)
#1 XInitExtension (libX11.so.6)
#2 XextAddDisplay (libXext.so.6)
#3 XScreenSaverQueryExtension (libXss.so.1+0x1565)
#4 x11_suspend_screensaver
#5 video_driver_init_internal
#6 drivers_init
#7 retroarch_main_init
The address 0x968 is a small offset off NULL -- libX11's
XQueryExtension dereferencing the Display * argument before
checking it for null.
Root cause:
gfx/common/x11_common.c::xss_screensaver_inhibit() was called
via x11_suspend_screensaver() with g_x11_dpy == NULL. The flow:
1. SDL2 video driver inits.
2. sdl2_set_handles() in gfx/common/sdl2_common.c:54 calls
video_driver_display_type_set(RARCH_DISPLAY_X11) so the
display *type* gate matches X11, and routes the SDL-owned
X Display through video_driver_display_set() at line 55.
3. The file-scope `g_x11_dpy` in x11_common.c:66 stays at its
initial NULL value. It's only assigned at line 795 inside
XOpenDisplay() in the xvideo / GL / X11-direct init paths,
which the SDL2 driver doesn't take.
4. drivers_init() -> video_driver_init_internal() ->
x11_suspend_screensaver() -- line 286 gates on display_type
== RARCH_DISPLAY_X11 (passes; SDL2 set it), then at lines
289-292 tries dbus_suspend_screensaver() if HAVE_DBUS, then
at line 293 calls xss_screensaver_inhibit(g_x11_dpy,
enable) -- with g_x11_dpy == NULL.
5. xss_screensaver_inhibit() at line 227 calls
XScreenSaverQueryExtension(NULL, ...) -> libX11
dereferences -> SEGV.
Why hasn't this been hit before?
Most desktop builds set HAVE_DBUS -- pkg-config finds dbus-1 on
basically every modern Linux distro -- so the
dbus_suspend_screensaver() call at line 290 short-circuits and
returns true before the xss path is reached. The CI runner
doesn't apt-install libdbus-1-dev (HAVE_DBUS off) but libxss1 is
pulled in transitively by xvfb / x11-utils (HAVE_XSCRNSAVER on
at link time), exposing the corner.
A symmetric defensive check already exists at line 255 in
xdg_screensaver_inhibit():
if (g_x11_dpy && g_x11_win)
...
The xss path was the missing-guard one.
* gfx/common/x11_common.c
Add a `if (!dpy) return false;` guard at the top of
xss_screensaver_inhibit(). Applies only to the
HAVE_XSCRNSAVER branch; the no-op stub at line 237 already
discards `dpy` with `(void) dpy;` and returns false.
Inline comment explaining the failure mode points at the
relevant call sites in sdl2_common.c so a future maintainer
can trace the invariant without re-deriving it.
Verified locally:
- The added check returns false for NULL dpy without
invoking libX11 (matches the existing "extension not
present" return contract). Non-NULL dpy paths are
unchanged.
- xss_screensaver_inhibit() is `static`, single caller (line
311 in x11_suspend_screensaver) -- no other reachable code
paths to audit.
Cannot verify locally that this resolves the CI SEGV without
rebuilding retroarch with the same apt set the runner has --
the next CI run is the experiment. Expected outcome:
xss_screensaver_inhibit() returns false harmlessly, the
function falls through to the xdg_screensaver path (which has
its own NULL guard), and the SDL2 smoke proceeds into
retro_run.1 parent e8c8bd1 commit 46ba4c1
1 file changed
Lines changed: 18 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
224 | 224 | | |
225 | 225 | | |
226 | 226 | | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
227 | 245 | | |
228 | 246 | | |
229 | 247 | | |
| |||
0 commit comments