Skip to content

Validate the server's view and cursor shapes, and bound the client zoom - #67

Merged
dhh merged 2 commits into
omacom:mainfrom
TechLuddite:fix/client-view-and-cursor-checks
Oct 24, 2026
Merged

dhh merged 2 commits into
omacom:mainfrom
TechLuddite:fix/client-view-and-cursor-checks

Conversation

@TechLuddite

@TechLuddite TechLuddite commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

A connected server could send a StreamConfig whose view was far smaller than its stream, for example 2x2 against 1920x1080. The client derives its integer decoder zoom from the view, so this asked for a zoom in the hundreds, and the CPU tier then tried to allocate terabytes and aborted. A server could also send a cursor hotspot outside the image. GTK returns no cursor for it, and the bindings wrap that NULL as a valid gdk::Cursor, which is undefined behaviour in a release build. Cursor payloads of up to 64 MiB were copied and queued before any check.

  • Both StreamConfig paths now build their geometry through one validating constructor that rejects a view that is zero, beyond the decoder limit, or less than half the stream. Servers always send the full-quality size the stream is scaled from, so half is conservative slack.
  • The worker clamps the zoom so a frame holds at most an 8K display's worth of pixels and no side past 8192. BgraFrame::zoomed uses checked arithmetic and reserves its buffer fallibly, showing the frame unzoomed when it cannot. A zoom the GPU refused is not retried every frame, and exportable_bgra frees its image and memory on its error paths. A StreamConfig that changes the stream size drops the zoom to 1, and the UI asks again for the new view.
  • Cursor shapes are read only when the payload is exactly widthheight4 with each side at most 1024. Anything else is skipped unread and treated as a shape that cannot be shown. Shapes coalesce into a single slot instead of queueing on the status channel, and remote_cursor rejects hotspots outside the image.

Validation:

  • New tests run the real worker session on the CPU tier against a fake server over an in-memory stream: a bad first or later view, a real encoded frame within the zoom bound, the zoom reset on a size change, a malformed cursor payload skipped while the next message is still answered, and cursor coalescing. Unit tests cover the zoom clamp, the GPU retry suppression, allocation failure and overflow in zoomed, and the hotspot bounds. Before the fix, the hotspot test panicked inside gdk4 and the zoomed test aborted on the allocation.
  • Live, against an unmodified v0.4.0 server on an RTX 3090 desktop (mirroring 2560x1440), this branch's gliff-probe stream-bench on an AMD Radeon 840M laptop ran within a frame per second of the v0.4.0 probe on both tiers, with no errors.
  • scripts/check.sh passes.

This covers the client StreamConfig bounds that #29 lists as still open. It conflicts with #47 and #80 in the client. I am happy to rebase whichever lands second.

Opus 5.5 high, on behalf of TechLuddite.
Implemented with Claude Opus 5.5 at high effort and reviewed by GPT-6.1 (Codex) at high reasoning effort over four passes, then re-reviewed on 2026-10-19 by Claude, GPT-6.1 and Grok 4.7 at medium effort.

TechLuddite and others added 2 commits October 15, 2026 21:38
A server could send a StreamConfig whose view was tiny next to its
stream, such as 2x2 for 1920x1080. The UI derives its integer zoom from
the view, so that asked the decoder for a zoom in the hundreds, and the
CPU tier then tried to allocate terabytes and aborted. Both StreamConfig
paths now build their geometry through Config::new, which rejects a view
that is zero, larger than the decoder limit, or less than half the
stream. Servers send the full-quality size the stream is scaled from, so
the factor is conservative slack.

The worker clamps the zoom so a frame holds at most an 8K display's worth
of pixels and no side past 8192. With the fixed GPU display ring and the
bounded frame channel, a server can then cost at most what a real 8K
session does. BgraFrame::zoomed uses checked arithmetic and reserves its
buffer fallibly, showing the frame unzoomed when it cannot. A zoom the GPU
decoder refused is not asked for again on every frame, and the display
images no longer leak their VkImage and memory when allocation or binding
fails. A StreamConfig that changes the stream size drops the zoom to 1,
and the UI forgets its last request so it asks again for the new view.

Cursor shapes are read only when the payload is exactly width*height*4
with both sides at most 1024. Anything else is skipped unread and passed
on empty, which the UI already treats as a shape it cannot show. Shapes
replace each other in a single slot instead of queueing on the status
channel. remote_cursor refuses a hotspot outside the image, for which GTK
returns no cursor that the bindings would wrap as a valid one.

Tests run the real worker session on the CPU tier against a fake server
over an in-memory stream, including a real encoded frame decoded at a
clamped zoom.
@greptile-apps

greptile-apps Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

The PR bounds server-supplied geometry and client zoom, and limits cursor memory use.

  • The client checks stream views and keeps enlarged frames within bounds.
  • The client checks remote cursor shapes before showing them.

No actionable issues were found. Tests were inspected but not run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Server message] --> B{Message kind}
    B -->|StreamConfig| C[Check stream and view sizes]
    C --> D[Update configuration and rebuild decoder if needed]
    B -->|VideoFrame| E[Bound requested zoom]
    E --> F[Decode and send frame]
    B -->|CursorShape| G[Read bounded pixels or skip payload]
    G --> H[Replace latest cursor shape]
    H --> I[UI takes shape]
    I --> J[Check hotspot before calling GTK]
Loading

Reviews (1) · Last reviewed commit: "Keep bounded decoder zoom across stream ..." · Reviewed by Greptile

@dhh
dhh merged commit a65bbb2 into omacom:main Oct 24, 2026
5 checks passed
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.

2 participants