From b7e054e94913abe16292cf9e08d848ccf46acdd9 Mon Sep 17 00:00:00 2001 From: Hermes Date: Thu, 5 Nov 2026 14:56:16 +0100 Subject: [PATCH] fix: build blank cursor from depth-1 bitmaps and check window creation (#3) --- internal/display/x11.go | 164 ++++++++++++++++++++++++++++------- internal/display/x11_test.go | 158 +++++++++++++++++++++++++++++++++ 2 files changed, 290 insertions(+), 32 deletions(-) diff --git a/internal/display/x11.go b/internal/display/x11.go index 7156d35..7cee954 100644 --- a/internal/display/x11.go +++ b/internal/display/x11.go @@ -188,39 +188,117 @@ func osdFallbackFontMetrics() (ascent, descent, cell int16) { // createWindowValues builds the CreateWindow value mask and values; the // order must follow the mask bit order (back pixel, event mask, cursor) so -// the server reads each value correctly. The blank cursor hides the pointer -// over the layer from its first frame, independently of video, wfb-ng or -// any cursor-hiding helper process. +// the server reads each value correctly. A zero cursor means cursor setup +// failed: the cursor bit is dropped entirely so the window maps with the +// server default cursor — an always-visible main window takes priority +// over cursor hiding. func createWindowValues(backPixel uint32, eventMask uint32, cursor xproto.Cursor) (uint32, []uint32) { + if cursor == 0 { + return xproto.CwBackPixel | xproto.CwEventMask, []uint32{backPixel, eventMask} + } values := []uint32{backPixel, eventMask, uint32(cursor)} return xproto.CwBackPixel | xproto.CwEventMask | xproto.CwCursor, values } -// newBlankCursor builds the invisible cursor from zeroed 1x1 source and -// mask pixmaps: an all-zero mask displays nothing regardless of the source -// contents, so the pointer disappears over the layer without any shell -// polling or helper process. The pixmaps and graphics context are freed -// immediately; the cursor itself lives until the session closes. -func newBlankCursor(conn *xgb.Conn, screen *xproto.ScreenInfo) xproto.Cursor { +// cursorBitmapDepth is the protocol-required depth of a cursor's source and +// mask bitmaps. Creating them at any other depth makes CreateCursor fail +// asynchronously — which previously poisoned the window id and surfaced +// only as a downstream BadWindow. +const cursorBitmapDepth = 1 + +// blankCursorX is the checked X-request surface of the blank cursor setup, +// so the sequence can be driven by tests that script failures at exact +// operations and pin request order and resource reclamation. +type blankCursorX interface { + CreatePixmapChecked(pid xproto.Pixmap, drawable xproto.Drawable, depth uint8, width, height uint16) error + CreateGCChecked(cid xproto.Gcontext, drawable xproto.Drawable) error + PolyFillRectangleChecked(drawable xproto.Drawable, gc xproto.Gcontext, rectangles []xproto.Rectangle) error + FreeGC(cid xproto.Gcontext) + CreateCursorChecked(cid xproto.Cursor, source, mask xproto.Pixmap) error + FreePixmap(pid xproto.Pixmap) +} + +type connCursorX struct{ conn *xgb.Conn } + +func (x connCursorX) CreatePixmapChecked(pid xproto.Pixmap, drawable xproto.Drawable, depth uint8, width, height uint16) error { + return xproto.CreatePixmapChecked(x.conn, depth, pid, drawable, width, height).Check() +} + +func (x connCursorX) CreateGCChecked(cid xproto.Gcontext, drawable xproto.Drawable) error { + return xproto.CreateGCChecked(x.conn, cid, drawable, 0, nil).Check() +} + +func (x connCursorX) PolyFillRectangleChecked(drawable xproto.Drawable, gc xproto.Gcontext, rectangles []xproto.Rectangle) error { + return xproto.PolyFillRectangleChecked(x.conn, drawable, gc, rectangles).Check() +} + +func (x connCursorX) FreeGC(cid xproto.Gcontext) { + xproto.FreeGC(x.conn, cid) +} + +func (x connCursorX) CreateCursorChecked(cid xproto.Cursor, source, mask xproto.Pixmap) error { + return xproto.CreateCursorChecked(x.conn, cid, source, mask, + 0, 0, 0, 0, 0, 0, 0, 0).Check() +} + +func (x connCursorX) FreePixmap(pid xproto.Pixmap) { + xproto.FreePixmap(x.conn, pid) +} + +// blankCursor allocates the ids for the blank cursor sequence and runs it +// over the connection. On failure the caller closes the connection, which +// reclaims every partially created resource. +func blankCursor(conn *xgb.Conn, screen *xproto.ScreenInfo) (xproto.Cursor, error) { pixmap, _ := xproto.NewPixmapId(conn) - xproto.CreatePixmap(conn, screen.RootDepth, pixmap, - xproto.Drawable(screen.Root), 1, 1) mask, _ := xproto.NewPixmapId(conn) - xproto.CreatePixmap(conn, screen.RootDepth, mask, - xproto.Drawable(screen.Root), 1, 1) gc, _ := xproto.NewGcontextId(conn) - xproto.CreateGC(conn, gc, xproto.Drawable(pixmap), 0, nil) - unit := []xproto.Rectangle{{Width: 1, Height: 1}} - xproto.PolyFillRectangle(conn, xproto.Drawable(pixmap), gc, unit) - xproto.PolyFillRectangle(conn, xproto.Drawable(mask), gc, unit) - xproto.FreeGC(conn, gc) cursor, _ := xproto.NewCursorId(conn) - // Zeroed colors and hotspot: with the all-zero mask nothing is ever - // displayed, so the fore/back colors never reach the screen. - xproto.CreateCursor(conn, cursor, pixmap, mask, 0, 0, 0, 0, 0, 0, 0, 0) - xproto.FreePixmap(conn, pixmap) - xproto.FreePixmap(conn, mask) - return cursor + return blankCursorOver(connCursorX{conn}, xproto.Drawable(screen.Root), + cursorBitmapDepth, pixmap, mask, gc, cursor) +} + +// blankCursorOver performs the blank-cursor resource sequence over the +// checked request surface: 1-bit source and mask bitmaps, a matching-depth +// graphics context, both bitmaps cleared to zero (an all-zero mask displays +// nothing regardless of the source), the cursor created, then the bitmaps +// and context reclaimed. Every request is checked at its own operation: the +// first failure stops the sequence, names the exact operation, frees what +// was already created, and returns before any window request runs. +func blankCursorOver(x blankCursorX, root xproto.Drawable, depth uint8, pixmap, mask xproto.Pixmap, gc xproto.Gcontext, cursor xproto.Cursor) (xproto.Cursor, error) { + if err := x.CreatePixmapChecked(pixmap, root, depth, 1, 1); err != nil { + return 0, fmt.Errorf("create cursor source bitmap: %w", err) + } + if err := x.CreatePixmapChecked(mask, root, depth, 1, 1); err != nil { + x.FreePixmap(pixmap) + return 0, fmt.Errorf("create cursor mask bitmap: %w", err) + } + if err := x.CreateGCChecked(gc, xproto.Drawable(pixmap)); err != nil { + x.FreePixmap(mask) + x.FreePixmap(pixmap) + return 0, fmt.Errorf("create cursor graphics context: %w", err) + } + unit := []xproto.Rectangle{{Width: 1, Height: 1}} + if err := x.PolyFillRectangleChecked(xproto.Drawable(pixmap), gc, unit); err != nil { + x.FreeGC(gc) + x.FreePixmap(mask) + x.FreePixmap(pixmap) + return 0, fmt.Errorf("clear cursor source bitmap: %w", err) + } + if err := x.PolyFillRectangleChecked(xproto.Drawable(mask), gc, unit); err != nil { + x.FreeGC(gc) + x.FreePixmap(mask) + x.FreePixmap(pixmap) + return 0, fmt.Errorf("clear cursor mask bitmap: %w", err) + } + x.FreeGC(gc) + if err := x.CreateCursorChecked(cursor, pixmap, mask); err != nil { + x.FreePixmap(mask) + x.FreePixmap(pixmap) + return 0, fmt.Errorf("create blank cursor: %w", err) + } + x.FreePixmap(pixmap) + x.FreePixmap(mask) + return cursor, nil } // osdTextRect is the output region covering one drawn text: its glyph cell @@ -346,14 +424,33 @@ func (o X11Opener) Open(ctx context.Context) (Session, error) { eventMask = osdEventMask } // The blank cursor hides the pointer over this layer from its first - // frame and stays on the layer across no-video/video transitions; the - // server reclaims it when the session's connection closes. - cursor := newBlankCursor(conn, screen) + // frame and stays on the layer across no-video/video transitions. It + // is optional: a cursor setup failure is surfaced as a bounded warning + // and the window still maps with the server default cursor — an + // always-visible main window takes priority over cursor hiding, and a + // cursor failure must never destroy or reconnect the display. + cursor, cursorErr := blankCursor(conn, screen) + if cursorErr != nil { + log.Printf("blank cursor unavailable (%v); mapping with the default cursor", cursorErr) + cursor = 0 + } valueMask, values := createWindowValues(screen.BlackPixel, eventMask, cursor) - xproto.CreateWindow(conn, screen.RootDepth, window, screen.Root, + // The window creation is checked at its own operation: a failure here + // is reported exactly, and no dependent request — properties, shapes, + // map, liveness — ever runs against an invalid window. + if err := xproto.CreateWindowChecked(conn, screen.RootDepth, window, screen.Root, 0, 0, screen.WidthInPixels, screen.HeightInPixels, 0, xproto.WindowClassInputOutput, screen.RootVisual, valueMask, - values) + values).Check(); err != nil { + // The window was never created: reclaim the connection (and the + // live cursor with a best-effort free) exactly like the other + // open failures, so reconnect attempts cannot leak either. + if cursor != 0 { + xproto.FreeCursor(conn, cursor) + } + conn.Close() + return nil, fmt.Errorf("create kiosk window: %w", err) + } // Tag the window as a Ground Station layer and announce its EWMH role // before mapping, so no observer or window manager ever sees an // untagged frame of our own layers. @@ -1039,9 +1136,12 @@ func foldToASCII(message string) string { } func (s *x11Session) Close() error { - // Free the blank cursor explicitly; closing the connection then makes - // the X server reclaim everything else, including the kiosk window. - xproto.FreeCursor(s.conn, s.cursor) + // Free the blank cursor when one was created; closing the connection + // then makes the X server reclaim everything else, including the kiosk + // window. + if s.cursor != 0 { + xproto.FreeCursor(s.conn, s.cursor) + } s.conn.Close() return nil } diff --git a/internal/display/x11_test.go b/internal/display/x11_test.go index fca180d..58fd132 100644 --- a/internal/display/x11_test.go +++ b/internal/display/x11_test.go @@ -3,6 +3,8 @@ package display import ( "context" "errors" + "fmt" + "strconv" "strings" "testing" "time" @@ -339,6 +341,162 @@ func TestFullscreenStateAtomsOrderByRole(t *testing.T) { } } +// scriptedCursorX scripts the checked blank-cursor request surface: it +// records every operation in order, can fail any single operation, and +// records reclaim calls — reproducing the production request-order and +// resource-lifecycle discipline without a server. +type scriptedCursorX struct { + failAt string + ops []string + freed []string + depths []uint8 +} + +func (s *scriptedCursorX) CreatePixmapChecked(pid xproto.Pixmap, _ xproto.Drawable, depth uint8, _, _ uint16) error { + s.depths = append(s.depths, depth) + s.ops = append(s.ops, "pixmap:"+strconv.FormatUint(uint64(pid), 10)) + if s.failAt == s.ops[len(s.ops)-1] { + return fmt.Errorf("bad pixmap request") + } + if s.failAt == "bitmap-depth" && depth != 1 { + return fmt.Errorf("cursor bitmaps must be depth 1") + } + return nil +} + +func (s *scriptedCursorX) CreateGCChecked(gc xproto.Gcontext, _ xproto.Drawable) error { + s.ops = append(s.ops, "gc") + if s.failAt == "gc" { + return fmt.Errorf("bad gc request") + } + return nil +} + +func (s *scriptedCursorX) PolyFillRectangleChecked(drawable xproto.Drawable, _ xproto.Gcontext, _ []xproto.Rectangle) error { + s.ops = append(s.ops, "fill:"+strconv.FormatUint(uint64(drawable), 10)) + if s.failAt == s.ops[len(s.ops)-1] { + return fmt.Errorf("bad fill request") + } + return nil +} + +func (s *scriptedCursorX) FreeGC(gc xproto.Gcontext) { + s.ops = append(s.ops, "free-gc") + s.freed = append(s.freed, "gc") +} + +func (s *scriptedCursorX) CreateCursorChecked(cid xproto.Cursor, _, _ xproto.Pixmap) error { + s.ops = append(s.ops, "cursor") + if s.failAt == "cursor" { + return fmt.Errorf("bad cursor request") + } + return nil +} + +func (s *scriptedCursorX) FreePixmap(pid xproto.Pixmap) { + s.freed = append(s.freed, "pixmap:"+strconv.FormatUint(uint64(pid), 10)) +} + +func TestBlankCursorBuildsCheckedDepthOneBitmapsInOrder(t *testing.T) { + // Cursor source and mask must be 1-bit bitmaps — the depth mismatch of + // the previous implementation is what made CreateCursor fail on the + // device and poisoned the window id. Every request is checked at its + // own operation, in order, and the pixmaps and context are reclaimed + // once the cursor exists. + x := &scriptedCursorX{} + cursor, err := blankCursorOver(x, 0x1, 1, 0x20, 0x21, 0x30, 0x40) + if err != nil { + t.Fatal(err) + } + if cursor != 0x40 { + t.Fatalf("cursor=%d want 0x40", cursor) + } + want := []string{"pixmap:32", "pixmap:33", "gc", "fill:32", "fill:33", "free-gc", "cursor"} + if strings.Join(x.ops, ",") != strings.Join(want, ",") { + t.Fatalf("ops=%v want %v", x.ops, want) + } + for _, depth := range x.depths { + if depth != 1 { + t.Fatalf("cursor bitmap depth=%d want 1", depth) + } + } + if strings.Join(x.freed, ",") != "gc,pixmap:32,pixmap:33" { + t.Fatalf("freed=%v want the context and both bitmaps reclaimed", x.freed) + } +} + +func TestBlankCursorRejectsNonBitmapDepths(t *testing.T) { + // The device regression: cursor bitmaps created at the root depth made + // the unchecked CreateCursor fail asynchronously, which poisoned the + // window id and surfaced only later as BadWindow on the shape request. + // The checked sequence must reject such a depth at its own operation. + x := &scriptedCursorX{failAt: "bitmap-depth"} + cursor, err := blankCursorOver(x, 0x1, 24, 0x20, 0x21, 0x30, 0x40) + if err == nil { + t.Fatal("a non-bitmap depth must fail the cursor sequence") + } + if cursor != 0 { + t.Fatal("a failed sequence must not return a cursor") + } + if len(x.ops) != 1 || x.ops[0] != "pixmap:32" { + t.Fatalf("ops=%v want the failure to stop the sequence at the first request", x.ops) + } +} + +func TestBlankCursorAttributesFailureToItsOwnOperation(t *testing.T) { + cases := []struct { + failAt string + want string + }{ + {"pixmap:32", "create cursor source bitmap"}, + {"pixmap:33", "create cursor mask bitmap"}, + {"gc", "create cursor graphics context"}, + {"fill:32", "clear cursor source bitmap"}, + {"fill:33", "clear cursor mask bitmap"}, + {"cursor", "create blank cursor"}, + } + for _, c := range cases { + x := &scriptedCursorX{failAt: c.failAt} + cursor, err := blankCursorOver(x, 0x1, 1, 0x20, 0x21, 0x30, 0x40) + if err == nil { + t.Fatalf("failAt=%q must fail", c.failAt) + } + if cursor != 0 { + t.Fatalf("failAt=%q must not return a cursor", c.failAt) + } + if !strings.Contains(err.Error(), c.want) { + t.Fatalf("failAt=%q error=%v want attribution %q", c.failAt, err, c.want) + } + // No operation after the failed one may run — resource reclamation + // (free-gc, pixmap frees) is the expected cleanup, not a dependent + // request. + for i, op := range x.ops { + if op == c.failAt { + for _, later := range x.ops[i+1:] { + if strings.HasPrefix(later, "free-") || strings.HasPrefix(later, "pixmap:") { + continue + } + t.Fatalf("failAt=%q: dependent operation ran after failure: %v", c.failAt, later) + } + break + } + } + } +} + +func TestCreateWindowValuesOmitCursorWhenUnavailable(t *testing.T) { + // A failed cursor setup must never poison the window: with no cursor, + // the window maps with the default cursor and the value list drops the + // cursor bit entirely. + mask, values := createWindowValues(0x100, 0x200, 0) + if mask != xproto.CwBackPixel|xproto.CwEventMask { + t.Fatalf("mask=%b want back pixel and event mask only", mask) + } + if len(values) != 2 || values[0] != 0x100 || values[1] != 0x200 { + t.Fatalf("values=%v want [backPixel eventMask]", values) + } +} + func TestTransparentShapesEmptyInputAndOutput(t *testing.T) { var input, output bool for _, kind := range transparentShapeKinds() {