fix(linux): guard XRandR NULL returns to avoid a SEGV during display setup - #5644
sresarehumantoo wants to merge 1 commit into
Conversation
…setup XRRGetScreenResources and XRRGetCrtcInfo both return NULL on a transient X error, which happens while the RandR configuration is still settling (for example when a second CRTC is being brought up as a multi-display session starts). Three call sites dereference the result without checking it, so the whole process dies instead of reporting a failure the caller can retry. Check all three: both x11_attr_t::init() and x11_display_names() bail out the way the adjacent xdisplay failures already do, and the CRTC lookup falls through to the existing "record the entire virtual desktop" path. Observed on a two-display session that raced CRTC bring-up, where sunshine would SEGV on session start and succeed on retry.
|
ReenigneArcher
left a comment
ReenigneArcher
left a comment
There was a problem hiding this comment.
The null checks address the segmentation faults, but two failure paths need recovery changes before this is safe to merge. Please add regression coverage for the null RandR responses and update the affected Doxygen behavior as required by the repository guidelines.
| crt_info = crtc_info_t {x11::rr::GetCrtcInfo(xdisplay.get(), screenr.get(), result->crtc)}; | ||
| } | ||
|
|
||
| if (crt_info) { |
There was a problem hiding this comment.
[P1] A transient null XRRGetCrtcInfo result for a successfully matched, selected monitor now takes the existing whole-desktop branch and returns success. Both X11 capture paths then use xattr.width/height from the root window, so a stream configured for one monitor can show every monitor for the lifetime of that capture object. Please fail initialization for this lookup error (as the screen-resources guard does) so the existing display reset/retry path can recover without changing the captured area.
| screen_res_t screenr {x11::rr::GetScreenResources(xdisplay.get(), xwindow)}; | ||
| if (!screenr) { | ||
| BOOST_LOG(error) << "Could not query X screen resources"sv; | ||
| return {}; |
There was a problem hiding this comment.
[P2] If XRRGetScreenResources is transiently null during startup, returning an empty list makes verify_x11() false. platf::init() performs that backend probe once and leaves sources[source::X11] unset, so a later healthy RandR state cannot be retried by a stream; Sunshine needs a restart to use X11. Please retry the transient discovery failure or provide a path to re-probe X11 availability.
This branch has not been deployed
2 participants
Description
XRRGetScreenResources and XRRGetCrtcInfo return NULL on a transient X error. There are three call sites in src/platform/linux/x11grab.cpp that dereference without checking:
Trigger: RandR config still settling, ex: a second CRTC comes up as a multi-display session starts. Result is a process-wide SEGV.
What I saw: sunshine SEGVs on session start with two displays, succeeds on retry. Found and fixed in my own tree in July; running on my test bench (vdi-test, Debian 13 (trixie)) since.
Verified on Ubuntu 22.04 WSL2 that it builds clean against master (393/393). Unit tests show no new failures (MouseHIDTest and EncoderTest fail identically before and after the patch in that environment, because it has no /dev/uinput; 0 failed tests either way).
Screenshot
Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage
See our AI usage policy.