Skip to content

Harden compat stack and hook violawww#7

Open
jserv wants to merge 1 commit into
mainfrom
violawww
Open

Harden compat stack and hook violawww#7
jserv wants to merge 1 commit into
mainfrom
violawww

Conversation

@jserv
Copy link
Copy Markdown
Contributor

@jserv jserv commented Jun 6, 2026

This consolidates differential test that gates the compat stack against the ViolaWWW browser. Affected Xlib surface spans XCopyArea, XGrabPointer, XQueryPointer, XWarpPointer, XGeometry/XWMGeometry, XDrawString family, XListFonts, XPutImage, XDoubleToFixed, and the SDL event/grab pipeline.

Memory safety and contract:

  • xCopyArea raster-op fallback for non-GXcopy/masked planeMask via read-modify-write through SDL_RenderReadPixels; source read before the destination renderer is resolved (lazy GET_RENDERER can switch the active SDL target); calloc-backed pixel buffers; viewport-offset translation on the read rects
  • subtractSegment uses caller-provided scratch buffer and int64 extents; renderFillRectClipByChildren allocations bounded against SIZE_MAX wrap
  • font.c NULL string guards; decodeChar2bString rejects negative counts and caps allocation at (SIZE_MAX-1)/3; XDrawString16 double-free removed; XmbTextPerCharExtents rejects negative buffer size; XmbTextEscapement / Xutf8TextEscapement use int64 fallback; text extents clamp to short range
  • decodeString stripped to bounded copy + NUL-terminate; no more escape interpretation that mangled file paths
  • XListFonts returns caller-owned strdup'd entries with NULL sentinel; XFreeFontNames walks-and-frees; isSupportedFontFileName case-insensitive
  • XGeometry / XWMGeometry pixel math via new satMulAdd (__builtin_mul_overflow / __builtin_add_overflow) with clampInt64ToInt narrowing
  • XCopyArea int64 extent guards before SDL_Rect math
  • XDoubleToFixed clamps representable range; NaN returns 0
  • XPutImage stride validation (non-negative bytes_per_line, minimum row size for ZPixmap-32 and XYBitmap fast paths)

Pointer + grab contract:

  • XQueryPointer NULL-checks every output, type-checks window, reports pressed-button bits in mask_return
  • XWarpPointer containment and warp coord math in int64
  • XGrabPointer returns BadWindow/GrabNotViewable for invalid args, AlreadyGrabbed on second active grab, GrabInvalidTime via CARD32 signed-delta against SDL_GetTicks (tick-wrap safe)
  • XGrabButton replaces existing grab on (button, modifiers, window) tuple match instead of leaking duplicates
  • Grab cursor saved/restored on ungrab; activatePassiveButtonGrab uses new getContainingWindow helper

Concurrency:

  • putBackEventsLock, trackedDisplaysLock, activePointerWindowLock (the last is shared by activePointerWindow + pointerButtonState); pointerButtonStateSnapshot() helper for motion/wheel reads
  • putImageScratchLock with lazy SDL_AtomicLock spinlock init, process-long lifetime; lock/unlock helpers pass the acquired SDL_mutex handle so a lazy-init race cannot make a thread unlock what it did not lock
  • Event pipe fds initialized to -1, closed on last display close; dead fdopen(FILE*) leak path removed; closeEventPipe runs unconditional last-display teardown when remainingDisplays == 0

Build:

  • Debug-only NULL guards on GET_WINDOW_STRUCT, GET_GC, GET_DISPLAY via extension statement expressions (release builds unchanged)
  • STRICT=1 build variant adds -Werror to first-party objects only, applied via mk/common.mk and mk/xcompat-libs.mk
  • New differential umbrella target runs motif-demos-check and violawww as separate sub-makes with set +e; aggregate exit reports both statuses on failure (one regression cannot mask the other)
  • New mk/violawww.mk pinning the bolknote/violawww commit; Makefile includes it; externals/violawww/ added to .gitignore

CI:

  • ViolaWWW build step in the motif job after demo smoke checks
  • if: !cancelled() so a demo failure does not skip ViolaWWW (and ccache stats); workflow cancellation still aborts
  • ViolaWWW build log uploaded as artifact on failure

Build commands run: make, make check, make
CFLAGS_EXTRA=-DDEBUG_LIBX11_COMPAT check, make STRICT=1.


Summary by cubic

Hardened the X11 compat stack and added a violawww differential build gate to catch regressions. This improves pointer/grab correctness, drawing/font safety, window show timing, and CI reliability.

  • Bug Fixes

    • Drawing/image: safer XCopyArea fallback with read rect clipping; int64 bounds on copies/geometry; XPutImage stride checks with a process-long scratch lock; default opaque alpha for X11 pixels; clamped XDoubleToFixed.
    • Fonts/text: reject invalid sizes/counts, clamp text extents, fix XDrawString16 double-free, sanitize decodeString, XListFonts returns caller-owned lists.
    • Pointer/grabs: NULL-safe XQueryPointer; int64 XWarpPointer; strict XGrabPointer/XGrabButton semantics (errors, AlreadyGrabbed, time checks), passive button grabs with cursor save/restore, release grabs on window destroy, and viewability-aware containment.
    • Concurrency/events: dedicated locks for event queues and XPutImage scratch; event pipe fds init to -1 and torn down on last display close.
    • APIs/behavior: overflow-safe XGeometry/XWMGeometry; XRenderSetPictureTransform stub; XGrabServer/XUngrabServer are no-ops; top-level windows start hidden and are shown on map.
  • Build and CI

    • STRICT=1 enables warnings-as-errors for first-party code via STRICT_CFLAGS; debug-only NULL guards for GET_DISPLAY, GET_GC, and GET_WINDOW_STRUCT.
    • New mk/violawww.mk and make violawww gate pinning bolknote/violawww; .gitignore ignores externals/violawww/; added make differential to run Motif smoke checks plus violawww.
    • CI builds violawww after Motif smoke tests, uploads logs on failure, and runs even if demos fail (if: !cancelled()).

Written for commit 0e9bc42. Summary will update on new commits.

Review in cubic

cubic-dev-ai[bot]

This comment was marked as resolved.

This consolidates differential gate that compiles the bolknote/violawww
browser against lib{x11,Xt,Xpm,Xmu}-compat + bundled Motif.

Memory safety and contract:
- xCopyArea raster-op fallback for non-GXcopy/masked planeMask:
  read-modify-write through SDL_RenderReadPixels with calloc-backed
  pixel buffers, viewport-offset translation on read rects, source
  read before destination renderer is resolved (GET_RENDERER can
  lazily switch active SDL target).
- subtractSegment uses caller-provided scratch buffer and int64
  extents; renderFillRectClipByChildren bounded against SIZE_MAX wrap.
- font.c NULL string guards; decodeChar2bString rejects negative
  counts and caps allocation; XDrawString16 double-free removed;
  XmbTextPerCharExtents rejects negative buffer size; Xmb/Xutf8
  TextEscapement use int64 fallback; text extents clamp to short.
- decodeString stripped to bounded copy + NUL-terminate.
- XListFonts returns caller-owned strdup'd entries with NULL
  sentinel; XFreeFontNames walks-and-frees; isSupportedFontFileName
  case-insensitive.
- XGeometry / XWMGeometry pixel math via new satMulAdd
  (__builtin_mul_overflow / __builtin_add_overflow) with
  clampInt64ToInt narrowing. Anchor logic preserves upstream X.org
  semantics: default-width anchoring when user spec has no position,
  merged-width anchoring when user position is set.
- XCopyArea int64 extent guards before SDL_Rect math.
- XDoubleToFixed clamps representable range; NaN returns 0.
- XPutImage stride validation: non-negative bytes_per_line plus a
  minimum row size for every format that dereferences image->data
  (ZPixmap any bpp, XYBitmap, XYPixmap).

Pointer + grab contract:
- XQueryPointer NULL-checks every output, type-checks window,
  reports pressed-button bits in mask_return.
- XWarpPointer containment and warp coord math in int64.
- XGrabPointer returns BadWindow / GrabNotViewable / AlreadyGrabbed
  / GrabInvalidTime (CARD32 signed-delta against SDL_GetTicks,
  tick-wrap safe).
- XGrabButton replaces existing grab on (button, modifiers, window)
  match instead of leaking duplicates; passive grabs are now released
  in destroyWindow so XID reuse cannot route events through a stale
  grab entry.
- Grab cursor saved/restored on ungrab; activatePassiveButtonGrab
  uses new getContainingWindow helper.

Window show timing:
- Top-level SDL_Windows are created with SDL_WINDOW_HIDDEN so the
  user never sees an empty frame; XMapWindow renders content first
  and only then calls SDL_ShowWindow. Reparent-into-top-level also
  shows after realize since it bypasses XMapWindow.

Concurrency:
- putBackEventsLock, trackedDisplaysLock, activePointerWindowLock
  (the last shared by activePointerWindow + pointerButtonState).
  pointerButtonStateSnapshot() snapshot helper for motion/wheel
  reads; press/release writes go through the same critical section.
- putImageScratchLock with lazy SDL_AtomicLock spinlock init,
  process-long lifetime, lock/unlock pair returns and accepts the
  acquired SDL_mutex* handle so the lazy-init race cannot make a
  thread unlock what it did not lock.
- Event pipe fds initialized to -1, closed on last display close;
  dead fdopen(FILE*) leak path removed; closeEventPipe runs
  unconditional last-display teardown.

Build system:
- Debug-only NULL guards on GET_WINDOW_STRUCT, GET_GC, GET_DISPLAY
  via __extension__ statement expressions (release builds unchanged).
- STRICT=1 build variant adds -Werror to first-party objects only,
  applied via mk/common.mk and mk/xcompat-libs.mk.
- New `differential` umbrella target runs motif-demos-check and
  violawww as separate sub-makes; aggregate exit reports both
  statuses on failure so one regression cannot mask the other.
- mk/violawww.mk pins the bolknote/violawww commit and fetches the
  pinned sha before checkout so commit bumps stay reproducible
  against a reused clone; externals/violawww/ added to .gitignore.

CI:
- ViolaWWW build step in the motif job after demo smoke checks,
  with `if: !cancelled()` so a demo failure does not skip ViolaWWW
  (and ccache stats).
- Build log uploaded as an artifact scoped to the violawww step's
  outcome — `if: steps.violawww.outcome == 'failure'` — so a prior
  motif-demos-check failure cannot attach a misleading successful
  log to the artifact.

Tests:
- XDoubleToFixed NaN/saturation; XGeometry massive-value clamp via
  parseable spec + huge font cell (avoids upstream XParseGeometry's
  internal signed-int overflow that UBSan caught in CI); XCopyArea
  INT_MAX rejection asserts BadValue via record_error; GXxor
  raster-op pixel readback; XListFonts pattern-buffer alias guard;
  XGrabPointer AlreadyGrabbed/BadWindow; ZPixmap XPutImage alpha
  promotion; X11-pixel-color pixmap and text rendering. The
  ClientMessage XSendEvent test now uses full XEvent storage so
  ASan's stack-buffer-overflow check is happy with the sizeof(XEvent)
  memcpy XSendEvent does on the caller buffer.

Deferred: full thread-safety beyond the four locks above (no other
shared state surfaced by reviewers); file splits for drawing.c clip
helpers and events.c pipe helpers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant