Skip to content

(master) Xephyr: consistently use pScreen->myNum for debug logging - #3723

Open
metux wants to merge 2 commits into
masterfrom
pr/master-xephyr-consistently-use-pscreen-mynum-for-debug-logging-_2026-09-23_17-26-02
Open

metux wants to merge 2 commits into
masterfrom
pr/master-xephyr-consistently-use-pscreen-mynum-for-debug-logging-_2026-09-23_17-26-02

Conversation

@metux

@metux metux commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

The debug logging is inconsistent with which screen number to print out in
(usually disabled) debug log messages. Just use pScreen->myNum here and
drop scrpriv->mynum entirely (not practically used anymore).

Signed-off-by: Enrico Weigelt, metux IT consult info@metux.net

@metux metux self-assigned this Sep 23, 2026
@metux
metux requested a review from a team September 23, 2026 15:27
metux added a commit that referenced this pull request Sep 23, 2026
The debug logging is inconsistent with which screen number to print out in
(usually disabled) debug log messages. Just use pScreen->myNum here and
drop scrpriv->mynum entirely (not practically used anymore).

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
PR: #3723
The debug logging is inconsistent with which screen number to print out in
(usually disabled) debug log messages. Just use pScreen->myNum here and
drop scrpriv->mynum entirely (not practically used anymore).

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux
metux force-pushed the pr/master-xephyr-consistently-use-pscreen-mynum-for-debug-logging-_2026-09-23_17-26-02 branch from b956a4f to 80c89a9 Compare September 25, 2026 16:58
screen->pScreen is NULL during hostx_init() because it's set later
during screen initialization. Use screen->mynum (the KdScreenInfo's
screen number) which is available from the start.

Also fix hostx_paint_rect for consistency.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux
metux force-pushed the pr/master-xephyr-consistently-use-pscreen-mynum-for-debug-logging-_2026-09-23_17-26-02 branch from 60a09d8 to 181d874 Compare September 25, 2026 18:58
@metux

metux commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Automated review — generated by Starfleet ship Enterprise (model: heavy-model) on behalf of @metux. Not a human review.

Verdict: pass — cleanup is fine; CI is currently NOT all-green (env/infra fails), so hold merge until green.

Code review

  • Removes the now-unused scrpriv->mynum and hostx_set_screen_number(), consolid fingers on
    screen->mynum (KdScreenInfo) / pScreen->myNum (Screen) — matching the PR goal.
  • NULL-safety of the new screen->pScreen->myNum derefs:
    • Under EPHYR_DBG (ephyrinit processScreenOrOutputArg, ephyrMapFramebuffer, debug motion logs):
      EPHYR_WANT_DEBUG is 0 → the macro compiles to do {} while (0), args never evaluated ⇒
      no runtime deref even if screen->pScreen is NULL at arg-parse/hostx_init time. Safe today.
    • The always-on deref (ephyrProcessKeyRelease: grabbed_screen = screen->pScreen->myNum) runs on a
      live screen (screen->pScreen is set) — safe.
  • hostx_set_win_title switches to screen->mynum (KdScreenInfo field, always set) instead of the
    removed scrpriv->mynum — that's the segfault-relevant part (no reliance on the dropped helper).

Advisory (non-blocking)

  • The three EPHYR_DBG derefs of screen->pScreen->myNum become live NULL-derefs the moment
    EPHYR_WANT_DEBUG is ever flipped on
    (e.g. during a debug session on a screen whose pScreen is
    not yet set). Suggest guarding them (screen->pScreen ? screen->pScreen->myNum : -1) to keep the
    debug path safe — not a blocker for the current build.

CI status — NOT mergeable yet (contradicts the "all green" report)

Current checks: 2 fail, 1 pending:

  • xserver-build-ubuntu-debug → environment/dependency-fetch failure (log: fatal: couldn't find remote ref refs/heads/rendercheck-1.6 / libdrm-2.4.121 / libxcvt-0.1.0 / xorgproto-2024.1) —
    infra (upstream refs/mirror), unrelated to this PR.
  • xserver-build-dragonflybsd → flaky VM-start (known).
  • 1 pending qemu lane.
    These must be re-run to green before merge.

Rules

  • Backport: N/A.
  • Driver-ABI: no impact (Xephyr/kdrive-internal).

@metux metux added the bot-review-passed Automated bot review found no blocking issues label Sep 25, 2026
@metux

metux commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

CI is NOT flaky — this PR introduces a real regressive NULL deref.

Symptom (ubuntu-debug, repro): all ephyr tests fail (xehypr/xephyr-glamor xdpyinfo/xrandr/xvinfo/XTS/go-xts/bug1354-*): Segmentation fault at address 0x0, "Server failed to start before setting up displayfd".

Root cause: the PR dereferences screen->pScreen->myNum in

  • ephyrMapFramebuffer (ephyr.c EPHYR_LOG line)
  • processScreenOrOutputArg (ephyrinit.c EPHYR_DBG line)

but screen->pScreen is NULL at that point:

KdInitScreens()  [kdrive.c]
  loop 1: KdInitScreen(screen)  -> card->cfuncs->scrinit()
    -> ephyrScreenInitialize()  -> ephyrMapFramebuffer(screen)   // pScreen NULL here
  loop 2: KdAddScreen(screen)   -> AddScreen(KdScreenInit)
    -> KdScreenInit sets  screen->pScreen = pScreen             // kdrive.c:866, only now

So both changed logging lines crash during early startup. The second commit's fix (hostx_set_win_title -> screen->mynum) is correct but leaves these two landmines.

Minimal fix: keep using the KdScreenInfo::mynum field (not screen->pScreen->myNum) at both changed log lines — the semantics are identical for diagnostics and pScreen is not yet available there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-review-passed Automated bot review found no blocking issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant