Conversation
A launch reuses whatever gateway it finds running and never asks which image it came from, so an edit to the gateway block reaches nothing until the running one is gone. There was no way to make that happen, and the way it was being done was to read the pid out of sandbox-gateway.json, kill it and remove the file by hand. stop is that by name. It signals the pid in the record, which is the sandbox's own init and not the bwrap that started it: killing the outer bwrap does not signal the sandbox nested below it, while the init is pid 1 of its own pid namespace and a SIGKILL from an ancestor namespace takes the whole namespace with it. That is the pid running.stop already names when it takes down a gateway that failed to come up. It reads the record through sandbox.Alive rather than looking at the pid, so a record left behind by a gateway that crashed reads as no gateway and nothing signals a number the kernel has since given to something else. It takes the same lock startOnce takes, because a launch between its check and its record would otherwise be left naming a process this has already killed. The session's namespace holder stays up. Only the gateway is reused across launches, so the holder is not what has to go, and taking it down would make the next launch rebuild a user namespace that was never the problem. The next launch starts a fresh gateway inside the same session. Being asked to stop a gateway that is not running is not a failure. It is the state the caller wanted, so it is reported and the exit status stays zero. A record left behind is removed on the way, because reap would have removed it had the launch that started the gateway still been there. status reports only what qubesome already holds: the configured image, the policy path resolved against the directory the config was read from, the subnet, the liveness of the holder and the gateway, readiness through the Ready call that already exists, and the addresses handed out. No new RPC, no proto change, and nothing that needs a newer gateway than the one that is running. Its readiness question is bounded at five seconds and not by the client's ten minutes. That deadline is sized for a launch, where Ready may be waiting on an image still being unpacked and waiting is the whole job. A status is there to report, so a gateway that does not answer becomes a line rather than a command that does not return. doctor bounds the same call for the same reason. Neither subcommand takes a profile, since there is one gateway per session. The config is still what names the image, the policy and the subnet, and it is reached the way doctor reaches one, through a running profile or the user-level file. A status that finds neither says so rather than printing three blank lines, and Inspect is handed the whole config rather than its gateway block so that a config which did not load and a config with no gateway block stay different answers. Reporting the second for the first is the bug doctor's session checks were fixed for. It is hidden, but not for the reason supervise and session-hold are. Nothing here runs inside a sandbox. It is hidden because qubesome manages the gateway itself, so the ordinary way to get one is to run a workload and there is nothing for a user to do here. Signed-off-by: Paulo Gomes <paulo@entire.io>
A profile stopped starting after a bubblewrap upgrade:
bwrap: Can't mount on symlink destination /etc/localtime
0.12.0 rewrote sandbox setup to resolve paths with
openat2(RESOLVE_IN_ROOT), fixing GHSA-pxhw-h44j-8pfx, and added with it
an unconditional refusal to mount on a destination that is a symlink. An
image ships /etc/localtime as one, so the bind qubesome has always asked
for is now fatal, for a workload as much as for a profile. Up to 0.11.x
ensure_file() tested the destination with stat(), which follows links,
so the mount landed on the image's link target instead and nobody had to
think about it. There is no opting out: --not-a-security-boundary only
reaches BIND_FAIL_OPEN, which is applied after the die.
So nothing is mounted on /etc/localtime any more. internal/util/tz
resolves the host's zone file, shares it under the name every image
knows it by, and TZ points the sandbox at it. That is the mechanism the
workload runner already used for profile.Timezone, and it carries the
zone name as well as the offsets, which following a link into the image
never did: a host on Europe/London used to reach a sandbox still calling
itself Etc/UTC. A host that copied its zone file into place rather than
linking to one has no name to carry, and keeps its offsets under one of
qubesome's own.
The cost is that /etc/localtime inside a sandbox is now the image's own,
so anything reading it while ignoring TZ sees the image's zone. Every C
library reads TZ ahead of that file, and so do the runtimes that resolve
a zone themselves rather than through one.
profile.Timezone now also reaches the profile sandbox, so the window
manager's own clock follows it rather than the host.
Assisted-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: 01M253CGA46V3B5FSJJDY0E4AN
qubesome host-run built the command's environment as exactly one entry,
DISPLAY, so it reached the profile's display with no cookie and no HOME
to find one under. The display is served by an Xwayland started with
-auth, which refuses a connection that arrives without one:
Authorization required, but no authorization protocol specified
The socket was never the problem. A profile bind-mounts the host's
/tmp/.X11-unix read-write, so Xwayland's socket has always been on the
host, under bwrap as much as under the container runners before it. Only
the cookie was missing, and it has been missing since the command was
added: nothing qubesome did ever authorized it. A profile whose dotfiles
loosened access control from the inside, with an xhost +local: or
+si:localuser:, would have papered over that, which is the likely reason
it used to work.
The client cookie is the one passed rather than the server's. Both carry
the same value, but the client copy is written with the wildcard family,
so it matches whatever the host currently calls itself, while the server
copy names the hostname captured when the profile started.
The host environment is now inherited rather than replaced. The command
runs on the host with the user's own privileges, so withholding HOME and
PATH from it isolates nothing and only stops it finding its own
configuration. WAYLAND_DISPLAY is dropped on the way through, because a
toolkit that finds one ignores DISPLAY, and the window would open on the
host desktop instead of in the profile.
Assisted-by: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: 01M25CV60KZCHXZCG7YG7VR27N
host-run waited on the command through CombinedOutput, so it held the terminal it was typed at until the application exited, and showed nothing until then. Launching an application into a profile is what the command is for, so that wait was its whole cost. It now starts the command and returns, as launching a workload does. Its stdout and stderr stay pointed at the terminal, because a command that cannot reach the profile's display says why there, and its stdin is left closed rather than pointed at a terminal the shell has taken back. The exit status now reports the launch rather than the application: a failure after it starts arrives as output, once the prompt is already back. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M25CVVJAM0PW6AGGC9FAFSGB
The gateway's stdout and stderr were this process's own. It is started by whichever qubesome run found none already running, and put in a session of its own so that a Ctrl-C at that terminal does not take the session's egress away with it, so what it says went to a terminal nobody is necessarily still watching, interleaved with the output of the workload that happened to start it. The audit line behind a refused connection is the one thing needed when a workload cannot reach something, and there was nowhere to go and read it. The gateway sandbox and its uplink now write to a log in the session directory instead. The sandbox's launch truncates it and the uplink, which is started once that sandbox is up, appends, so one file describes the gateway that is running, in order. It is 0600: it holds every name a workload asked for and every host it was allowed or refused, which is a record of what the user was doing. Truncating on launch is why there is no rotation. The log belongs to one gateway rather than to the session, and a gateway is only ever replaced by the first one having gone. `qubesome gateway logs` reads it back, with -n for the last lines and -f to keep printing what is added. A follow is a poll, since the sandbox writes to the file through a descriptor and nothing notifies a reader of it, and it copies in bounded steps so a chatty gateway cannot make the reader hold the whole log. A session that never started a gateway has no log, and saying so is the answer to what was asked rather than a failure to answer it. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M25TRK55EZ9D3Q8SB0701QFB
An application launched with host-run opened on the profile's default workspace rather than the one being looked at. host-run used to give the command DISPLAY and nothing else. It now hands over the whole host environment, which is right for HOME and PATH and wrong for the handful of variables that name the session the command was typed or bound in, because the session it is about to appear in is a different one. DESKTOP_STARTUP_ID is the one that moved the window. A window manager that spawns through startup notification records the workspace it spawned from against the id it exports, and the application passes that id to the next window it opens as _NET_STARTUP_ID. The profile's window manager then reads an id for a launch it never saw and places the window by whatever that resolves to. It is single use as well: it belongs to the launch that created it and to no later one, which is why awesome unsets it itself when it spawns without a context of its own. XDG_ACTIVATION_TOKEN is the Wayland spelling of the same thing and goes with it. WAYLAND_DISPLAY was already being dropped, for a reason that is the same one told differently, so the three are now one list with the reason written once. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M26DCGXTE5NCSVHSXA82MPNR
A profile came up on a layout its user does not type on, again. Asking localectl first was right for the host it was written for and wrong for the one it broke. The two tools disagree more often than they look like they should, and which of them to believe depends on where the question is asked, not on a preference between them. On X11 the running answer wins. setxkbmap asks the X server the user is typing on, which includes a layout applied by hand after login, and that is the layout the profile should come up on. localectl reports what was configured, which on a host whose layout is set at runtime is a layout nobody is using. On Wayland the configured answer wins, which is what the previous behaviour was reaching for. There setxkbmap reaches Xwayland, which carries its own default rather than the compositor's keymap, so it answers confidently with nobody's layout. An unset session type is treated as X11: every X11 session sets it, and a session that sets nothing is not a Wayland one. XKB_DEFAULT_ still wins over both. The two tests that asserted the old order called firstOf directly with a fixed order, under names describing a policy that is now session dependent. What replaces them drives the decision itself, on both sessions, and covers the fallback in both directions. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M26DERTN4KQDGNQD5BQZFBA4
The window manager was given the profile's cookie and had WAYLAND_DISPLAY taken away from it, and was then left to whatever DISPLAY xwayland-run exports. The profile container's own DISPLAY is the host's, because that is what the compositor presents through, and the whole of /tmp/.X11-unix is mounted so that its socket is reachable. Anything under the window manager that inherited that would be talking to the host session rather than to the profile. It is named here instead, beside the two that were already being set, so nothing in that subtree can reach the host's X server by inheriting a path to it. Workloads were never exposed this way: each mounts only its own socket rather than the directory. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M26DGJC18YYEP4XZ3G07QR7R
A session gateway serves every workload of every profile, so its log is every decision made for all of them interleaved. Finding why one workload could not reach something meant reading past everything else. -profile and -workload narrow it. The gateway knows a workload only by the name qubesome registered, which is the workload's own name and its profile's joined by a dash, and either half may hold a dash of its own, so the pair cannot be split back into two names with any certainty. Naming both is therefore the exact question and naming one alone is a prefix or a suffix of it, which is spelled out in the command's help rather than left to surprise someone. -n now counts the lines it shows rather than the lines in the file. Asking for one workload's last twenty otherwise gives however few of its lines happen to fall in the file's last twenty. Reading the log is line oriented rather than a byte copy, which is what a filter needs. A follow holds a line back until it has a newline on it: a line is what carries the workload a decision was about, so half of one cannot be matched, and printing it unmatched would show another workload's log to someone who asked not to see it. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M26DJVEWJG0V1SSAY0QJD1CM
doctor said whether the gateway was running and whether it was ready, and nothing about the thing it exists to do. A workload that cannot reach something got a clean bill of health and no hint of where to look. The new check separates two findings that are not the same. A refused connection is the policy working, so it is reported at ok, with the count and the hosts named, because "why can I not reach this" is what brings someone to doctor and the answer is a host name. An error is something the gateway tried to do and could not, so it warns and names the most recent one, which is where a splice that failed to dial or an original destination it could not recover now shows up. The log is summarised in internal/gateway rather than read here, so the one place that already understands the gateway's log format is the only place that parses it. It parses tolerantly: the format belongs to a component on its own release cycle, so a line that cannot be read contributes nothing rather than failing the check. The readiness check's advice was to read the gateway's own output, which until recently went to a terminal that was usually gone. It names the command now. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M26DMN1PHXHQET4H4S6H5R4Y
gateway status read its config through profileConfigOrDefault(""), which
takes an active profile's config only when exactly one profile is active
and otherwise falls back to the user-level file. The gateway is session
wide: it was started by whichever launch found none running, from that
profile's config, and may have come from any of the active ones. With more
than one active, status could therefore name an image, a policy and a
subnet the running gateway has nothing to do with, and say nothing to
suggest it was guessing. Raised in review on #226.
Several active profiles are usually not an ambiguity at all. One qubesome
config commonly defines several profiles, so the run dir holds several
symlinks to one file, and resolving them leaves a single config that is
the answer whichever profile started the gateway. That is now what is
looked for.
It is only profiles started from genuinely different files that leave
nothing here able to say which the gateway came from, and status now says
that instead of picking one. ConfigProblem already exists for exactly this
shape of answer, and reporting that it cannot be told is the behaviour the
command was written around: it reports a reason rather than leaving the
lines blank.
What this does not do is say what the running gateway is actually running,
as opposed to what a config names. Only the gateway's own launch knows
that, and nothing records it. The allocation record already carries the
subnet the running gateway hands addresses out of, and is already compared
against the config, so it is where the image and the policy path would go
if that becomes worth having.
Assisted-by: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: 01M26F8MJDW9GE2ZX2Z8XKNECR
The gateway drops every port but 80, 443 and 53, so a workload reaches
anything else by asking it for a tunnel instead of connecting out. Asking
means naming an endpoint, and nothing in the sandbox said what it was.
The gateway's own address a workload could have worked out for itself: it
is already its default route and the nameserver in its resolv.conf. The
port it could not, because that one belongs to the gateway image. So what
it is told is the pair, in QUBESOME_GATEWAY_PROXY, ready to be used as it
stands. An ssh client reaches a host it is allowed to reach with
ProxyCommand socat - PROXY:$QUBESOME_GATEWAY_PROXY:%h:%p
and nothing there names a port that is the gateway's business to choose.
The port is a const beside the gateway image's other paths, and carries
the same caveat they do: it belongs to the image, so the two have to be
changed together. Telling a workload the whole endpoint rather than only
the address is what keeps that caveat out of anybody's dotfiles.
A workload with no gateway is told nothing rather than told an empty
value, which would read as an endpoint that exists and is nothing.
Assisted-by: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: 01M26FTJHYYKTQB58PQ5NEECRV
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL: a writable handle from OpenFile closed without handling the error, where a failure can mean data that never reached the disk. Nothing here writes through this descriptor. It is opened, handed to the sandbox and to the uplink, which get their own from Start onwards, and qubesome's copy is closed as being of no further use. So unlike the write in replace there is no last part of one arriving at close, and nothing to lose by not looking. Discarding the error is still the wrong shape. A close that fails says the filesystem holding the log is unwell, and the log is the file a gateway that goes wrong explains itself in, so it is worth a line. It is a warning rather than an error because the gateway is already running by then, and a log qubesome could not close is no reason to take one down. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M26P1R6XJFJ64N08Y768H75Q
Stop signalled the sandbox and removed its record in the same breath. A delivered signal is not a sandbox that has gone, so the next launch could take the lock, find no record, and start a replacement while the old pid namespace, its outer bwrap and its uplink were still coming down. Raised in review on #226. It now waits for the record to stop naming something running, and only then removes it. The record has to outlive the wait, because it is what the next launch decides by. Waiting is a poll, since there is nothing to wait on: a gateway is not the child of whatever stops it, so it cannot be reaped here and the kernel will not report its going. Five seconds bounds it, which is a ceiling on the kernel finishing rather than an estimate: SIGKILL to pid 1 of a pid namespace takes the namespace with it and is not something the process can put off. sandbox.Exited is new and is not the negation of Alive. A process killed by something that is not its parent stays in the table as a zombie, with its /proc entry and its start time intact, so Alive reads it as running when it is only waiting to be collected. The parent that would collect it is usually a qubesome run that exited at its terminal long ago, so a wait on Alive would have waited out the whole grace every time. It reads the state character from the same stat line the start time already comes from. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M26PCGB8A1QDXH7RYXWA20XB
The subnet mismatch reported "the running gateway hands addresses out of X", which is false in the state it is most likely to be read in. The record outlives the gateway that wrote it, and gateway stop leaves exactly that behind: nothing running, and a note of the range the last one handed addresses out of. The test covering it had no gateway running either. Raised in review on #226. It now describes the record, and says what to do about it, which is the same remedy a launch is given when it refuses the same mismatch. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M26PD3GTGSJB8AR4615X32WN
A launch and a status report the same mismatch, and said it differently. The status one was reworded to describe the record rather than a gateway, because a status is read in the state gateway stop leaves: nothing running, and a note of the range the last one used. This one opens the same way now, so the two are recognisably about the same thing. What it keeps is the claim the status message had to drop. Allocate is only ever reached through Attached, after Up, so a gateway is running by the time this is read, and its holding the first address of the old range is the reason the remedy is a restart rather than an edit. Dropping that here would have made the message consistent and less useful. The message had no test. It has one now, since it is the whole of what a user is told when a subnet changes under a session that has already handed addresses out. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 01M26PK5W39Z73BV7D6PJNJTNG
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain in gateway shutdown, log handling, and configuration or diagnostic attribution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request improves profile-aware execution and display/input propagation while adding gateway logging, lifecycle controls, and egress diagnostics.
Changes:
- Routes host-run and profile windows to the correct display.
- Propagates keyboard layout, timezone, and gateway proxy settings.
- Adds gateway logs, status/stop commands, summaries, and doctor checks.
File summaries
| File | Reviewed change and final review note |
|---|---|
internal/util/xkb/xkb.go |
Selects keyboard layout based on the session type. |
internal/util/xkb/xkb_test.go |
Tests layout source preference and fallback. |
internal/util/tz/tz.go |
Resolves host timezone data for sandboxes. Final finding: nit (1 vote) on the malformed validation sentence. |
internal/util/tz/tz_test.go |
Tests timezone resolution. |
internal/runners/bwrap/testdata/supervised.golden |
Updates supervised Bubblewrap expectations. |
internal/runners/bwrap/testdata/plain.golden |
Updates plain Bubblewrap expectations. |
internal/runners/bwrap/testdata/hostnet.golden |
Updates host-network Bubblewrap expectations. |
internal/runners/bwrap/testdata/granted.golden |
Updates granted-access Bubblewrap expectations. |
internal/runners/bwrap/spec.go |
Adds timezone mounts and gateway proxy environment. |
internal/runners/bwrap/run.go |
Resolves timezone and proxy inputs for workloads. |
internal/runners/bwrap/run_test.go |
Tests updated sandbox inputs. |
internal/runners/bwrap/proxyenv_test.go |
Tests gateway proxy environment behavior. |
internal/profiles/profiles.go |
Propagates profile timezone and session settings. |
internal/profiles/profiles_test.go |
Tests profile environment propagation. |
internal/profiles/display.go |
Targets the profile display explicitly. |
internal/profiles/display_test.go |
Tests display environment and arguments. |
internal/gateway/summary.go |
Summarizes gateway decisions and errors. |
internal/gateway/summary_test.go |
Tests gateway summaries. |
internal/gateway/stop.go |
Implements gateway shutdown. Final finding: critical (1 vote) because teardown is not coordinated before restart. |
internal/gateway/stop_test.go |
Tests gateway stopping behavior. |
internal/gateway/status.go |
Reports gateway configuration and runtime state. |
internal/gateway/status_test.go |
Tests gateway status reporting. |
internal/gateway/run.go |
Redirects gateway output and records lifecycle state. |
internal/gateway/proxyaddr_test.go |
Tests proxy endpoint calculation. |
internal/gateway/logs.go |
Reads, filters, tails, and follows logs. Findings: critical (1 vote) for unbounded -n allocation, also at line 241; moderate (1 vote) for unbounded unterminated-line allocation, O(total lines × K) tailing, and missing retention limits; nit (1 vote) for the misleading missing-log error. |
internal/gateway/logs_test.go |
Tests gateway log handling. |
internal/gateway/logline.go |
Parses and filters gateway log fields. Final finding: moderate (1 vote) because quoted escape sequences are not decoded. |
internal/gateway/logline_test.go |
Tests log parsing and selectors. |
internal/gateway/attach.go |
Computes workload gateway proxy endpoints. |
internal/files/files.go |
Adds session gateway log paths. |
internal/doctor/session.go |
Adds gateway egress diagnostics. Final finding: moderate (1 vote) because ambiguous config resolution can suppress the active gateway checks. |
internal/doctor/session_test.go |
Tests session diagnostics. |
internal/doctor/environment_test.go |
Extends doctor environment support. |
internal/doctor/env.go |
Reads gateway log summaries. |
cmd/cli/root.go |
Registers gateway commands. |
cmd/cli/host_run.go |
Filters session variables and launches host commands asynchronously. Final finding: moderate (1 vote) because the Wayland session type is preserved instead of forcing X11. |
cmd/cli/host_run_test.go |
Tests host-run environment filtering. |
cmd/cli/gateway.go |
Implements gateway status, stop, and logs commands. Findings: moderate (3 votes) for falling back to unrelated user config, and moderate (1 vote) for silently omitting unresolved active profile links. |
cmd/cli/gateway_test.go |
Tests gateway configuration selection. |
Review details
Suppressed comments (11)
cmd/cli/gateway.go:106
- This silently discards an active profile whose config symlink cannot be resolved. If another profile resolves, the code can treat that one config as authoritative; if all fail, it reaches the unrelated fallback below. Treat any unresolved active link as unknown/ambiguous instead of omitting it, otherwise
gateway statuscan attribute the running gateway to the wrong config.
target, err := filepath.EvalSymlinks(path)
if err != nil {
continue
cmd/cli/host_run.go:49
hostRunEnvremovesWAYLAND_DISPLAY, but it preserves the launching session'sXDG_SESSION_TYPE=wayland. On a Wayland host this still lets GUI toolkits select the host Wayland backend and bypass the profile's X server; the profile environment itself explicitly forcesXDG_SESSION_TYPE=X11ininternal/profiles/profiles.go:861. Set the session type to X11 along withDISPLAYso host-run is consistently directed to the profile display.
return append(env,
"DISPLAY=:"+strconv.Itoa(int(display)),
"XAUTHORITY="+cookie,
)
internal/doctor/session.go:40
- This branch treats the caller's config as authoritative, but bare
doctorstill loads it throughprofileConfigOrDefault(""); when multiple active profiles come from different configs, that helper falls through to the user-level config. If that fallback has no gateway (or a different one), this early return suppresses the real session gateway and egress checks even when a gateway is running. The session check needs an explicit unknown/ambiguous-config path, or the CLI must use the same session-config resolution as gateway status.
checks := []Check{holder, gateway}
if gateway.Status == OK {
// Readiness and egress only say something once there is a
// gateway to ask about. Piling a second failure on top of the
// same cause would bury the one worth reading.
checks = append(checks, checkGatewayReady(env), checkGatewayEgress(env))
internal/gateway/logline.go:60
logValuerecognizes quoted slog fields but does not decode their escape sequences; it drops the backslash and copies only the following byte. An error such asmsg="dial failed\nretry"is therefore summarized asdial failednretry(and escaped tabs or other characters are also corrupted), sodoctorcan report the wrongLastError. Decode the complete quoted token with the same escaping rules as the slog/text format before returning it.
case '\\':
if i+1 < len(rest) {
i++
b.WriteByte(rest[i])
internal/gateway/logs.go:182
ReadStringreads and allocates the entire unterminated suffix before returningio.EOF, so themaxLineLencheck runs only after an arbitrarily large chunk has already been allocated. A long partial gateway line can therefore makegateway logs -fconsume unbounded memory despite the documented limit; read bounded fragments and retain an explicit oversized-line discard state across follow ticks.
for {
internal/gateway/logs.go:241
-fkeeps an open descriptor and leaves its reader at the old EOF. Truncating this same inode when a new gateway starts does not reset that reader, so a follow running acrossgateway stopand the next launch skips the new gateway's initial output until enough bytes pass the old offset (and may miss it entirely). The follower needs to detect truncation and seek/reset before reading the new log.
func openLog(path string) (*os.File, error) {
internal/gateway/logs.go:241
- The gateway sandbox writes through the descriptor installed as
cmd.Stdout/cmd.Stderr, while the uplink opens the same path withO_APPENDinappendLog. This descriptor is not append-only, so after the uplink appends a record the gateway's stale file offset can overwrite it and lose the very diagnostics this log is meant to preserve. Keep the initial truncation but also open this descriptor withO_APPENDso both writers append atomically.
func openLog(path string) (*os.File, error) {
internal/gateway/logs.go:124
- This is described as a ring, but it shifts every retained line for every input line.
gateway logs -n Ktherefore becomes O(total_lines*K), which can make a large session log very slow; use a circular index or queue so eviction is O(1).
if len(ring) == last {
ring = append(ring[:0], ring[1:]...)
internal/gateway/logs.go:70
- This error says the session never started a gateway, but a gateway started by an older qubesome version can be running without the new log;
doctoralready treats that as a missing-log case. The message should describe the unavailable log rather than incorrectly asserting that no gateway was started.
if errors.Is(err, os.ErrNotExist) {
internal/gateway/logs.go:224
- The session log is opened in append mode for the uplink and is only truncated when a new gateway starts; no size or retention bound is applied. A long-lived or busy browser session can therefore accumulate one line per proxy decision until it consumes the user's run-directory disk space. Add a bounded/rotated log policy (or another explicit retention limit) before making this the persistent gateway output.
// explaining a gateway that had trouble coming up.
func appendLog(path string) (*os.File, error) {
f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_APPEND, files.FileMode)
if err != nil {
internal/util/tz/tz.go:29
- The sentence "A path that does not match names no zone" is grammatically malformed and obscures the validation rule. Rephrase it to state that a path which does not match a zone name does not name a zone.
- Files reviewed: 42/42 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+101
to
+103
| if last > 0 { | ||
| ring = make([]string, 0, last) | ||
| } |
Comment on lines
+124
to
+126
| // No profile running, or one whose config no longer reads. The | ||
| // user-level file is the only thing left that describes a gateway. | ||
| return profileConfigOrDefault(""), "" |
There was a problem hiding this comment.
🔵 Needs a closer look
Six moderate findings remain unresolved across gateway status, host-run isolation, log following, gateway cleanup, and timezone mounting.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
cmd/cli/host_run.go:48
- Removing
WAYLAND_DISPLAYalone does not prevent a Wayland client from connecting to the host compositor: libwayland falls back to$XDG_RUNTIME_DIR/wayland-0when the variable is unset, and this function deliberately preserves bothXDG_RUNTIME_DIRand the hostXDG_SESSION_TYPE. On a Wayland host, a GTK/Qt command can therefore still ignoreDISPLAY=:...and open on the host desktop instead of the profile. Force the launched command onto X11 and make the host Wayland socket unavailable in this environment.
cmd/cli/gateway.go:126
- This fallback guesses the user-level config whenever no active config symlink can be resolved. Ordinary local starts do not create an entry in
activeConfigs, and a gateway also outlives the profile that started it, sogateway statuscan report the user config's image, policy, and subnet for a gateway started from a project or Git config. Persist the source config with the gateway when it starts, or report the configuration as unknown instead of inferring it here.
// No profile running, or one whose config no longer reads. The
// user-level file is the only thing left that describes a gateway.
return profileConfigOrDefault(""), ""
internal/gateway/logs.go:109
bufio.Scannerreturns the final unterminated token as a line. If the gateway is writing a record while the initial read runs, this emits only the prefix and leavesfollowat EOF; the remainder is then treated as a new line, so filters can split or permanently miss that record. Preserve an unterminated tail and combine it with the next read, asfollowOncealready does.
scanner := bufio.NewScanner(f)
scanner.Buffer(make([]byte, 0, bufio.MaxScanTokenSize), maxLineLen)
for scanner.Scan() {
internal/gateway/logs.go:170
- Follow keeps one file descriptor and its offset for the entire command, but restarting the gateway truncates this same log path with
O_TRUNCand writes the new log from offset 0. The follower then remains past the new EOF and skips the new gateway's initial records (only resuming after the file grows beyond the old offset). Detect truncation and reset/reopen the reader when the file shrinks.
func follow(ctx context.Context, w io.Writer, f *os.File, selects func(string) bool) error {
ticker := time.NewTicker(followInterval)
defer ticker.Stop()
reader := bufio.NewReader(f)
var pending strings.Builder
for {
select {
case <-ctx.Done():
// The only way a follow ends is by being asked to stop, so
// that is not an error to report.
return nil
case <-ticker.C:
if err := followOnce(w, reader, &pending, selects); err != nil {
internal/gateway/stop.go:86
waitGoneonly observes the inner sandbox PID; thereapgoroutine still waits for the outer bwrap and then unconditionally removes this shared state path. Once this method removes the record and releases the lock, a replacement gateway can start and write a new record before the old reaper runs, after which the old reaper deletes the replacement's state. Coordinate with the outer-process reaper or make cleanup conditional on the PID/start time that it originally recorded.
if err := waitGone(g.StatePath, stopGrace, stopPoll); err != nil {
return st.PID, err
}
// A qubesome run at a terminal exits long before the gateway it started
internal/util/tz/tz.go:47
SandboxPathis a nested file under/usr/share/zoneinfo, but the workload and profile mount builders pass it directly to bubblewrap without creating the destination. An image without the host's exact zone file (common for minimal images) makes the bind fail and prevents the sandbox from starting, so the new timezone propagation does not work for those images. Create the destination in the sandbox setup or mount the host file at a guaranteed existing path before settingTZ.
- Files reviewed: 42/42 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Running things in a profile
host-runno longer carries the launching session'sDESKTOP_STARTUP_IDandXDG_ACTIVATION_TOKENinto the profile. They were placing windows on theprofile's default workspace instead of the one being looked at.
host-runreturns once the command starts, and is authorised against theprofile's display.
DISPLAYby name ratherthan inheriting the host's, which the profile container holds so the
compositor can present through it.
localectlon Wayland, instead of always preferring the configured one. Aprofile was coming up on a layout its user does not type on.
TZrather than a mount.Seeing what the gateway is doing
start it, read back with
qubesome gateway logs, narrowed with-profileand
-workload.qubesome doctorreports the gateway's egress: refused hosts are named atok, since a denial is the policy working, and gateway errors warn.gateway statusno longer reports a config the running gateway may not havecome from.