rootless: Fix SIGBUS when copying 8bit window contents upwards - #13
Open
tenox7 wants to merge 17 commits into
Open
rootless: Fix SIGBUS when copying 8bit window contents upwards#13tenox7 wants to merge 17 commits into
tenox7 wants to merge 17 commits into
Conversation
Failure to do so causes an overvlow in glxClientCallback
Application Specific Information:
X.Org X Server 1.18.99.1 Build Date: 20160911
=================================================================
==52118==ERROR: AddressSanitizer: SEGV on unknown address 0x000102b27b80 (pc 0x000103433245 bp 0x70000de67c20 sp 0x70000de67c00 T6)
#0 0x103433244 in __asan::asan_free(void*, __sanitizer::BufferedStackTrace*, __asan::AllocType) (libclang_rt.asan_osx_dynamic.dylib+0x3244)
XQuartz#1 0x10347aeee in wrap_free (libclang_rt.asan_osx_dynamic.dylib+0x4aeee)
XQuartz#2 0x102e6a5ed in glxClientCallback glxext.c:301
XQuartz#3 0x102b672a3 in _CallCallbacks dixutils.c:737
XQuartz#4 0x102b2f0c6 in CallCallbacks callback.h:83
XQuartz#5 0x102b5c15a in NextAvailableClient dispatch.c:3562
XQuartz#6 0x102d7060c in AllocNewConnection connection.c:777
XQuartz#7 0x102d71355 in EstablishNewConnections connection.c:863
XQuartz#8 0x102b662f0 in ProcessWorkQueue dixutils.c:523
XQuartz#9 0x102d52a7f in WaitForSomething WaitFor.c:175
XQuartz#10 0x102b204f6 in Dispatch dispatch.c:411
XQuartz#11 0x102b61e01 in dix_main main.c:301
XQuartz#12 0x10254c42a in server_thread quartzStartup.c:66
XQuartz#13 0x7fffc5f16aaa in _pthread_body (libsystem_pthread.dylib+0x3aaa)
#14 0x7fffc5f169f6 in _pthread_start (libsystem_pthread.dylib+0x39f6)
#15 0x7fffc5f161fc in thread_start (libsystem_pthread.dylib+0x31fc)
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
(cherry picked from commit 1d22931)
Failure to do so causes an overvlow in RRClientCallback().
=================================================================
==41262==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000103ccfbc8 at pc 0x0001034f32b9 bp 0x7000035a94c0 sp 0x7000035a94b8
WRITE of size 4 at 0x000103ccfbc8 thread T6
#0 0x1034f32b8 in RRClientCallback randr.c:72
XQuartz#1 0x1038c75e3 in _CallCallbacks dixutils.c:737
XQuartz#2 0x10388f406 in CallCallbacks callback.h:83
XQuartz#3 0x1038bc49a in NextAvailableClient dispatch.c:3562
XQuartz#4 0x103ad094c in AllocNewConnection connection.c:777
XQuartz#5 0x103ad1695 in EstablishNewConnections connection.c:863
XQuartz#6 0x1038c6630 in ProcessWorkQueue dixutils.c:523
XQuartz#7 0x103ab2dbf in WaitForSomething WaitFor.c:175
XQuartz#8 0x103880836 in Dispatch dispatch.c:411
XQuartz#9 0x1038c2141 in dix_main main.c:301
XQuartz#10 0x1032ac75a in server_thread quartzStartup.c:66
XQuartz#11 0x7fffc5f16aaa in _pthread_body (libsystem_pthread.dylib+0x3aaa)
XQuartz#12 0x7fffc5f169f6 in _pthread_start (libsystem_pthread.dylib+0x39f6)
XQuartz#13 0x7fffc5f161fc in thread_start (libsystem_pthread.dylib+0x31fc)
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
(cherry picked from commit f923500)
Workaround a performance issue that this introduces in XQuartz Fixes: XQuartz/XQuartz#166 This reverts commit ac7a4bf. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
The glyph bounding box in RootlessGlyphs is computed in drawable-relative (window-local) coordinates, but RootlessDamageBox expects global (screen) coordinates. Without translating by drawable.x/y, the damage region often fell outside the window's borderClip and was silently discarded by RootlessDamageRegion. RootlessGlyphs accumulated glyph positions starting from (xSrc, ySrc) instead of (0, 0). The xSrc/ySrc parameters are the source picture reference point, not destination offsets. Every standard Glyphs implementation (fbGlyphs, miGlyphs) starts position accumulation from (0, 0) and builds up destination coordinates by adding each GlyphList's xOff/yOff deltas. Starting from xSrc shifted the entire damage bounding box by (xSrc, ySrc), causing it to miss the actual rendered region. For typical text rendering where xSrc equals the destination x position, this doubled the offset, placing the damage box well outside the window's borderClip and causing RootlessDamageRegion to silently discard it. Fixes [1/2]: XQuartz/XQuartz#323 Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
RootlessStopDrawing() finds the frame to release by walking up from a window with TopLevelParent(), which is wrong for callers whose window no longer resolves to the frame it still owns. Once a framed window has been reparented below another framed window, that lookup finds either a different, live frame -- releasing the wrong one and leaving the intended one held -- or no frame at all, in which case it silently does nothing. Split out an entry point that names the frame directly so the next commits can release a frame from those paths. Naming the frame alone is sufficient because winRec->win is assigned in exactly one place, immediately adjacent to the SETWINREC() it pairs with, so a published frame record always points back at its own window. That is also what makes this behavior-preserving for the existing callers, since winRec->win is then the TopLevelParent() the old code computed. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Xplugin refuses to destroy a window whose backing store is still locked for drawing, and XQuartz escalates that refusal to FatalError(), so the whole session dies when a client closes a window. The rootless layer holds the drawing lock across requests and releases it lazily from the block handler, so every frame operation that has to talk to the implementation releases it first -- with three exceptions this series fixes, of which frame destruction is the one that kills the server, and whose documented contract has always claimed otherwise. Two paths reach the destroy with the lock still held. Reconfiguring a window after it was unrealized re-locks the frame, and the damage that would normally arm the block handler is clipped away against the emptied borderClip, so nothing ever releases it again; a window manager that restores a client's border width while withdrawing it does exactly this. Leaving rooted mode also destroys the root's frame, which is always realized and routinely drawn into. Updates are deliberately not flushed, matching how unrealizing a window releases the lock, so the contract is corrected rather than satisfied. Fixes: XQuartz/XQuartz#451 Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
When a framed window is reparented below another window that has no frame of its own, the frame is handed over rather than destroyed, but nothing released the drawing lock first. The implementation rejects the unmap that follows on a locked frame and the return value is discarded, so the frame silently stays mapped. The resize after it was already covered, but only because reshaping the new parent happens to release the lock once the frame record has moved across; releasing it explicitly up front no longer depends on that ordering. Flushing rather than discarding matters here because the pending damage is expressed relative to the frame's current geometry, which the hand-off is about to rewrite. For a colour-indexed frame the implementation also re-lays-out the client-side backing store on the resize without adjusting the recorded damage, so damage left behind would afterwards describe stale coordinates over relocated content. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Both the frame hash and the queue that guards it were assigned inside assert(), so a build that defines NDEBUG drops the assignments and leaves them null for the life of the server, trapping on the first frame that is created. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
The handler for a native window having been moved dereferences the window it is given without checking it, unlike its sibling that handles native state changes. The implementation looks the window up from an Xplugin window id and has always been able to come up empty, for instance once the frame has been destroyed but a queued move event for it has not yet been drained. The next commit makes that lookup return nothing reliably rather than occasionally, so guard the handler to match its sibling before it becomes easy to hit. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
The hash mapping Xplugin window ids back to frame records is updated from asynchronously dispatched blocks, but the rootless layer frees the record as soon as the frame is destroyed, so both updates can run against freed memory. The removal leaves the hash handing out a freed record until the block is scheduled. The insertion is worse: it reads the record to obtain the key, so a frame created and destroyed within one turn of the queue files the record under a garbage id that the matching removal then cannot find, stranding it in the hash indefinitely. Doing both synchronously keeps the hash from ever describing a frame that no longer exists. Nothing else uses the queue in a way that could deadlock, and frames are created and destroyed rarely enough for the cost not to matter. The lookup also let the record escape its critical section before reading the window out of it, which is what actually exposed callers on the AppKit thread; reading it inside closes that. The window it returns may still be stale, but only the server thread ever dereferences one, and it is the thread that would have freed it. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Failing to hand a window back to the window server is recoverable, so terminating the X server over it is wildly disproportionate: every connected client loses its unsaved work because we could not release one window. This is the same judgement already made for the matching unlock failure, which has been logged rather than fatal for over a decade. The cost is worse than a plain memory leak and worth stating: Xplugin bails out before deregistering the window, so it stays registered, locked, and possibly still on screen for the life of the server, while we forget its id entirely. That is still preferable to killing the session. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Resizing the root window takes a separate path from every other resize, and that path reconfigured the frame without releasing the drawing lock first. In rooted mode the root has a frame and is routinely drawn into, so a display reconfiguration could be rejected outright by the implementation and leave the root's frame at the old size, with the error discarded. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Xplugin rejects any attempt to reconfigure a frame that is locked for drawing, and every caller discarded the result, so a move, resize, restack, reshape or unmap could be dropped without a trace and leave a window on screen that the X server believed it had already dealt with. That silence is much of why the fatal destroy failure took so long to pin down. Folding the variant that honours the native-reconfiguration guard into the same wrapper keeps that from being the one path with no diagnostic, and records which hooks the guard actually covers. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Moving a frame releases the drawing lock without flushing, and has done for as long as the two callers have existed, so the documented contract has never matched the code. Left alone it would now be the only stale copy of a sentence corrected for the hook just above it. Resizing is deliberately left as documented: with the root's frame and the reparent hand-off both flushing, every caller now really does flush first. Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
The rootless screen pixmap is a single scanline with a zero devKind, so every row aliases row 0. That bounds vertical addressing, but fb derives addresses from the composite clip alone and never looks at the pixmap's size, so nothing bounds the horizontal term. Rootless deliberately leaves top-level windows unclipped by the root, which is only safe while a window has a frame to draw into. A window without one still points at the screen pixmap, and window coordinates are signed, so any client drawing to it reads and writes outside the allocation in either direction. Bound these clips where they are computed, so that every consumer inherits it -- clipList and borderClip also drive Render, expose handling and CopyWindow, none of which involve a GC. Clamp again during ValidateGC to cover a screen resize, which republishes the pixmap without recomputing any clip but the root's. Fixes: XQuartz/XQuartz#466 Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
…ames libXplugin backs XP_DEPTH_INDEX8 windows with a software backing store of its own, and its _xp_backing_scroll() computes the destination address of the copy with a 32bit (bytes_per_row * dy) multiply which is then zero extended. Any xp_copy_window() moving content upwards is therefore done ~4GB past the backing store and takes the server down: Exception Type: EXC_BAD_ACCESS (SIGBUS) Exception Codes: KERN_PROTECTION_FAILURE at 0x00000002283111e0 0 libsystem_platform.dylib _platform_memmove + 168 1 CoreGraphics CGBlt_copyBytes + 496 2 libXplugin.1.dylib _xp_backing_scroll + 156 3 libXplugin.1.dylib xp_copy_window + 516 4 X11.bin RootlessCopyWindow + 752 5 X11.bin DRICopyWindow + 180 6 X11.bin miSpriteCopyWindow + 168 7 X11.bin miMoveWindow + 388 This only affects 8bit frames; every other depth is scrolled by the window server instead. Nothing is lost by doing those copies ourselves, since libXplugin would only memcpy them as well.
Since 74b8383 ("rootless: Use screen_x and screen_y instead of pixmap pointer hacks") RootlessCopyWindow() damages rgnDst after translating it into the pixmap's coordinate space, while RootlessDamageRegion() expects global coordinates and intersects with pWin->borderClip. For any frame which isn't at the screen origin the intersection comes up empty, so the copied contents are never flushed to the screen. Translate the region back before damaging it.
There was a problem hiding this comment.
Pull request overview
This PR addresses two rootless/XQuartz regressions in RootlessCopyWindow() that affect 8-bit (depth 8) operation: (1) it avoids calling the platform “accelerated” CopyWindow path for 8-bit framed windows to prevent a known SIGBUS in libXplugin’s backing-store scroll logic, and (2) it fixes damage reporting after software copies by ensuring the damaged region is provided in global coordinates.
Changes:
- Add
RootlessCanAccelerateCopy()and use it to preventimp->CopyWindowfrom being used for depth-8 framed windows (falling back to server-side copy instead). - Fix region coordinate space after
miCopyRegion()by translatingrgnDstback to global coordinates before callingRootlessDamageRegion().
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jeremyhu
force-pushed
the
server-26.1-apple
branch
from
September 3, 2026 03:20
cab18f6 to
92dab3d
Compare
jeremyhu
added a commit
that referenced
this pull request
Sep 3, 2026
Failure to do so causes an overvlow in glxClientCallback
Application Specific Information:
X.Org X Server 1.18.99.1 Build Date: 20160911
=================================================================
==52118==ERROR: AddressSanitizer: SEGV on unknown address 0x000102b27b80 (pc 0x000103433245 bp 0x70000de67c20 sp 0x70000de67c00 T6)
#0 0x103433244 in __asan::asan_free(void*, __sanitizer::BufferedStackTrace*, __asan::AllocType) (libclang_rt.asan_osx_dynamic.dylib+0x3244)
#1 0x10347aeee in wrap_free (libclang_rt.asan_osx_dynamic.dylib+0x4aeee)
#2 0x102e6a5ed in glxClientCallback glxext.c:301
#3 0x102b672a3 in _CallCallbacks dixutils.c:737
#4 0x102b2f0c6 in CallCallbacks callback.h:83
#5 0x102b5c15a in NextAvailableClient dispatch.c:3562
#6 0x102d7060c in AllocNewConnection connection.c:777
#7 0x102d71355 in EstablishNewConnections connection.c:863
#8 0x102b662f0 in ProcessWorkQueue dixutils.c:523
#9 0x102d52a7f in WaitForSomething WaitFor.c:175
#10 0x102b204f6 in Dispatch dispatch.c:411
#11 0x102b61e01 in dix_main main.c:301
#12 0x10254c42a in server_thread quartzStartup.c:66
#13 0x7fffc5f16aaa in _pthread_body (libsystem_pthread.dylib+0x3aaa)
#14 0x7fffc5f169f6 in _pthread_start (libsystem_pthread.dylib+0x39f6)
#15 0x7fffc5f161fc in thread_start (libsystem_pthread.dylib+0x31fc)
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
(cherry picked from commit 1d22931)
jeremyhu
added a commit
that referenced
this pull request
Sep 3, 2026
Failure to do so causes an overvlow in RRClientCallback().
=================================================================
==41262==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000103ccfbc8 at pc 0x0001034f32b9 bp 0x7000035a94c0 sp 0x7000035a94b8
WRITE of size 4 at 0x000103ccfbc8 thread T6
#0 0x1034f32b8 in RRClientCallback randr.c:72
#1 0x1038c75e3 in _CallCallbacks dixutils.c:737
#2 0x10388f406 in CallCallbacks callback.h:83
#3 0x1038bc49a in NextAvailableClient dispatch.c:3562
#4 0x103ad094c in AllocNewConnection connection.c:777
#5 0x103ad1695 in EstablishNewConnections connection.c:863
#6 0x1038c6630 in ProcessWorkQueue dixutils.c:523
#7 0x103ab2dbf in WaitForSomething WaitFor.c:175
#8 0x103880836 in Dispatch dispatch.c:411
#9 0x1038c2141 in dix_main main.c:301
#10 0x1032ac75a in server_thread quartzStartup.c:66
#11 0x7fffc5f16aaa in _pthread_body (libsystem_pthread.dylib+0x3aaa)
#12 0x7fffc5f169f6 in _pthread_start (libsystem_pthread.dylib+0x39f6)
#13 0x7fffc5f161fc in thread_start (libsystem_pthread.dylib+0x31fc)
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
(cherry picked from commit f923500)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reopens #10, which was left stranded on
server-21.1-applewhen XQuartz moved to 26.1. Both bugs are still present onserver-26.1-apple, unchanged.XQuartz in 8bit mode (
defaults write org.xquartz.X11 depth 8) dies with SIGBUS whenever a child window is moved upwards and the copy exceedsxp_scroll_area_threshold:_xp_backing_scroll()computes the destination assrc + (uint32)(bytes_per_row * dy) + dx. The multiply is 32bit and gets zero extended, so a negativedylands ~4GB past the backing store:In the report above:
dst - src == 0xFFFFF81F == (uint32)(-1952) + (-65), matching a 1952 byte stride, 1 row up, 65 px left.Only
XP_DEPTH_INDEX8frames reach that code, everything else is scrolled by the window server, which is why this is 8bit only. So don't hand 8bit frames to the implementation'sCopyWindow— libXplugin would only memcpy them anyway, so nothing is lost.Second commit is a separate bug in the same function: since 74b8383 the region is damaged after being translated out of global coordinates, so
RootlessDamageRegion()intersects it againstborderClipand comes up empty for any frame not at the screen origin, and the copied contents are never flushed. It matters more now that 8bit copies always take this path.Analysis is from the crash report plus libXplugin disassembly; not build tested yet.