fix(connector): reject out-of-range server desktop sizes by mmorrissette-devolutions · Pull Request #2084 · Devolutions/IronRDP

Mathieu Morrissette (mmorrissette-devolutions) · 2026-10-06T17:24:34Z

Summary

During capabilities exchange, the connector adopted the desktop size from the server's Bitmap capability set without validation. Clients then allocated their framebuffer from it in DecodedImage::new, using unchecked multiplication and vec![0; len]. A server announcing a very large size (up to 65535x65535) could make the client abort on allocation failure, and on wasm32 the multiplication can also overflow.

The connector now rejects a server desktop size with a zero dimension, or with a dimension above 32766, the virtual desktop limit that servers enforce through ERRINFO_VIRTUALDESKTOPTOOLARGE. The connection fails with a clear error during capabilities exchange.

With both dimensions bounded, the framebuffer size computed in DecodedImage::new can no longer overflow, on wasm32 included, and stays within what the protocol allows. The existing vec![0; len] allocation is kept unchanged, so memory is still committed lazily as the screen is drawn.

A valid size can still exceed what wasm32 can allocate: 32766x32766 at 32 bpp is above isize::MAX there. ironrdp-web now probes the framebuffer allocation with try_reserve_exact before creating the image, and fails the session with an error instead of aborting. wasm32 is single-threaded, so the real allocation reuses the memory the probe just freed. This matches FreeRDP, which treats a failed framebuffer allocation as a connection error.

Native clients (ironrdp-client, FFI) keep the infallible vec![0; len]. On native targets, this PR therefore reduces the maximum allocation from ~17 GB to ~4.3 GB rather than eliminating the abort. A host that cannot provide a framebuffer of the maximum valid size can still abort. Making DecodedImage::new fallible would close this and is left for a follow-up.

Tests

New tests in ironrdp-testsuite-core: demand_active_accepts_server_desktop_size_within_range and demand_active_rejects_server_desktop_size_out_of_range.
cargo test -p ironrdp-testsuite-core -p ironrdp-session passes.
cargo clippy -p ironrdp-web --target wasm32-unknown-unknown passes.

Compatibility

No server can legitimately advertise a desktop size above 32766:

Windows (RDS): larger virtual desktops are refused with ERRINFO_VIRTUALDESKTOPTOOLARGE ("larger than the maximum allowed value of 32,766").
xrdp: defines CLIENT_MONITOR_DATA_MAXIMUM_VIRTUAL_DESKTOP_WIDTH/HEIGHT as 0x7FFE (32766) and refuses any virtual desktop outside its min/max range (libxrdp/libxrdp.c). Its Bitmap capability sends back the size from the client's request (libxrdp/xrdp_caps.c).
ironrdp-acceptor: clamps to 200..=8192 before advertising.
FreeRDP: as a server, it refuses a zero width or height from the client (libfreerdp/core/gcc.c). As a client, it takes the server's Bitmap size as-is, the same gap this PR closes here.

A zero dimension is the only edge case, and I found no server that advertises one. Before this change, a zero size produced an empty framebuffer in the native client, and ironrdp-web already failed with "desktop width is zero", so rejecting it is consistent with the web client.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0d4e270e7f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Benoît Cortier (CBenoit) · 2026-10-07T13:20:56Z

~~PR automation is failing because of a picky-krb 0.12.5 incompatibility, fixed on master by #2074. Please rebase on master to fix it.~~

Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes.

github-actions

PR 2084 rejects server desktop sizes outside 1..=32766 during capabilities exchange, with boundary tests, and adds a wasm32 framebuffer allocation probe in ironrdp-web before canvas and image creation. Verified against pr-head: the connector check (connection_activation.rs:297-306) sits after desktop-size resolution including the client-config fallback (283-295); the probe helper (session.rs:1929-1940) is called before Canvas::new (673-689) and before the reactivation DecodedImage::new (1081-1082). The core fix is correct and protocol-sound. All three valid specialist candidates were confirmed independently and are accepted unchanged; none blocks the change.

github-actions · 2026-10-07T15:06:03Z

+fn ensure_framebuffer_allocatable(width: u16, height: u16) -> anyhow::Result<()> {
+    let len = usize::from(width)
+        .checked_mul(usize::from(height))
+        .and_then(|pixels| pixels.checked_mul(usize::from(PixelFormat::RgbA32.bytes_per_pixel())))
+        .with_context(|| format!("framebuffer size overflows for a {width}x{height} desktop"))?;
+
+    Vec::<u8>::new()
+        .try_reserve_exact(len)
+        .with_context(|| format!("not enough memory for a {width}x{height} desktop framebuffer"))?;
+
+    Ok(())
+}


[skeptical] Framebuffer allocation probe does not guarantee the subsequent infallible allocation succeeds — medium 🟠 — ensure_framebuffer_allocatable validates a single W*H*4 reservation via try_reserve_exact and immediately frees it, but the infallible vec![0; len] in DecodedImage::new (ironrdp-session/src/image.rs:147-151) runs while other large buffers are live: Canvas::new allocates the canvas backing store between the probe and the image on the initial path (673-689), and in the Deactivation-Reactivation path (1081-1082) the old image is only dropped after the new one is constructed, so peak usage is roughly twice what the probe validated. Under memory pressure the wasm32 abort path remains reachable, notably on resolution change with a large previous framebuffer resident. The helper also duplicates DecodedImage::new's size formula and will drift if the layout changes. The probe is an acceptable stopgap, but the residual abort risk should be documented; a fallible DecodedImage constructor or reset would close it fully.

github-actions · 2026-10-07T15:06:03Z

+                if !(1..=MAX_DESKTOP_DIM).contains(&desktop_size.width)
+                    || !(1..=MAX_DESKTOP_DIM).contains(&desktop_size.height)
+                {
+                    return Err(reason_err!(
+                        "ConnectionActivation::CapabilitiesExchange",
+                        "server desktop size {}x{} is outside the supported range",
+                        desktop_size.width,
+                        desktop_size.height
+                    ));
+                }


[skeptical] Desktop size range check also gates the client's own configuration and the error blames the server — low 🟡 — The negotiated desktop_size falls back to self.config.desktop_size when the server sends no Bitmap capability set (existing lines 283-295), so the added 1..=32766 check also rejects out-of-range client-side configuration, a case the PR does not describe, and the message 'server desktop size {}x{} is outside the supported range' incorrectly attributes a client-supplied value to the server. Impact is narrow: it needs a non-compliant server that omits the Bitmap capability plus a misconfigured client, and failing fast on a zero-size config improves on the previous empty-framebuffer behavior. Restricting the check to the server-provided Bitmap size or adjusting the message for the fallback case would resolve it.

github-actions · 2026-10-07T15:06:03Z

+fn step_demand_active_with_desktop_size(width: u16, height: u16) -> ironrdp_connector::ConnectorResult<Written> {
+    let mut demand_active = SERVER_DEMAND_ACTIVE.clone();
+    let bitmap = demand_active
+        .pdu
+        .capability_sets
+        .iter_mut()
+        .find_map(|capability_set| match capability_set {
+            CapabilitySet::Bitmap(bitmap) => Some(bitmap),
+            _ => None,
+        })
+        .expect("server demand active should include a bitmap capability");
+    bitmap.desktop_width = width;
+    bitmap.desktop_height = height;
+
+    let mut sequence = ConnectionActivationSequence::new(test_config(), IO_CHANNEL_ID, USER_CHANNEL_ID);
+    let mut output = WriteBuf::new();
+    let frame = encode_server_share_control(ShareControlPdu::ServerDemandActive(demand_active));
+    sequence.step(&frame, None, &mut output)
+}


[code-compressor] New test helper repeats existing demand-active step scaffolding — low 🟡 — step_demand_active_with_desktop_size repeats the scaffolding of the pre-existing demand_active_static_channel_chunk_size (96-123): clone SERVER_DEMAND_ACTIVE, find_map one capability set out of the PDU, mutate it, build a ConnectionActivationSequence with the same test_config()/channel IDs, encode via encode_server_share_control, and sequence.step(...). A single closure-parameterized helper (fn step_demand_active_with(mutate: impl FnOnce(&mut DemandActivePdu))) would replace both, removing roughly eight duplicated lines and a second divergent copy of the sequence-bootstrapping recipe without changing test behavior. Tradeoff: the file's existing convention is inline scaffolding (a third variant at 342-364 repeats parts of it), so this is an optional consolidation with no runtime impact.

The desktop size advertised in the server's Bitmap capability set was adopted as-is, and clients allocate their framebuffer from it. A server announcing 65535x65535 made the client attempt a ~17 GB allocation, and on wasm32 the size computation could overflow. Reject sizes with a zero dimension or a dimension above 32766, the virtual desktop limit servers enforce through ERRINFO_VIRTUALDESKTOPTOOLARGE. This bounds the framebuffer to ~4.3 GB. A valid size can still exceed what wasm32 can allocate, so the web client now probes the framebuffer allocation and fails the session instead of aborting. Native clients keep the infallible allocation and can still abort on hosts that cannot provide a framebuffer of that size.

github-actions

The PR adds a 1..=32766 per-dimension validation of the server-announced desktop size in the connector's Capabilities Exchange, plus an allocation probe (try_reserve_exact) before framebuffer creation in ironrdp-web, with tests for both accept and reject paths. The core fix is sound: it removes unbounded server-controlled allocations and the wasm32 multiplication overflow, and the wasm probe converts hard aborts into session errors. Verified residual concerns: the 32766 ceiling is a client convention stricter than MS-RDPBCGR (which leaves bitmap desktopWidth/Height unconstrained), the probe ignores the still-live old framebuffer during reactivation resize so aborts remain possible on wasm32, the check also validates the client's own config fallback under a 'server desktop size' error label, native clients still abort on the maximum valid size via infallible DecodedImage::new, and a minor API simplification of the probe helper is available. All five candidates verified accurate; publis…

github-actions · 2026-10-07T15:45:19Z

+                if !(1..=MAX_DESKTOP_DIM).contains(&desktop_size.width)
+                    || !(1..=MAX_DESKTOP_DIM).contains(&desktop_size.height)
+                {
+                    return Err(reason_err!(
+                        "ConnectionActivation::CapabilitiesExchange",
+                        "server desktop size {}x{} is outside the supported range",
+                        desktop_size.width,
+                        desktop_size.height
+                    ));
+                }


[protocol] Client drops connections on server desktop sizes above 32766, a bound not required by MS-RDPBCGR — medium 🟠 — The new check aborts Capabilities Exchange when the Demand Active bitmap capability advertises width or height of 32767..65535, but MS-RDPBCGR defines TS_BITMAP_CAPABILITYSET.desktopWidth/desktopHeight as unconstrained u16 and its client-side Demand Active processing rules contain no desktop-size range validation. The 32766 limit is a client convention backed by real server behavior (ERRINFO_VIRTUALDESKTOPTOOLARGE, xrdp 0x7FFE, ironrdp-acceptor's 8192 clamp) and documented by the PR, yet it remains a stricter-than-specified interop restriction that can terminate connections with spec-conformant third-party servers advertising large desktops. Zero-dimension rejection is a defensible policy; the upper bound deserves explicit documentation as a product limit.

github-actions · 2026-10-07T15:45:19Z

                            } = connection_activation.connection_activation_state()
                            {
                                debug!("Deactivation-Reactivation Sequence completed");
+                                ensure_framebuffer_allocatable(desktop_size.width, desktop_size.height)?;


[skeptical] Allocation probe runs while the old framebuffer is still alive, so resize can still abort on wasm32 — medium 🟠 — In the Deactivation-Reactivation path, ensure_framebuffer_allocatable(desktop_size.width, desktop_size.height) runs immediately before image = DecodedImage::new(...), which allocates the new framebuffer while the previous image is still held (dropped only after the assignment). The probe checks the new size in isolation, so on wasm32 a resize between two large valid desktops can still exceed linear memory and abort, precisely in the scenario the probe was added for. The probe is a heuristic; the complete fix is a fallible DecodedImage::new, deferred by the author. The probe should account for the live framebuffer or this residual window should be documented.

github-actions · 2026-10-07T15:45:19Z

+                if !(1..=MAX_DESKTOP_DIM).contains(&desktop_size.width)
+                    || !(1..=MAX_DESKTOP_DIM).contains(&desktop_size.height)
+                {
+                    return Err(reason_err!(
+                        "ConnectionActivation::CapabilitiesExchange",
+                        "server desktop size {}x{} is outside the supported range",
+                        desktop_size.width,
+                        desktop_size.height
+                    ));
+                }


[skeptical] Range check also validates the client's own config size but reports it as a server error — low 🟡 — When the Demand Active carries no Bitmap capability set, desktop_size falls back to self.config.desktop_size, and the new check then rejects the client's own requested size. Config::desktop_size has no documented constraint and is sent unvalidated in TS_UD_CS_CORE, so a caller configuring e.g. 65535x65535 now fails at activation with 'server desktop size ... is outside the supported range' even though the server never announced that value. The rejection is defensible (the server could not honor such a request anyway), but the error misattributes the value to the server and surfaces late; validate config at construction or broaden the message.

github-actions · 2026-10-07T15:45:19Z

+                if !(1..=MAX_DESKTOP_DIM).contains(&desktop_size.width)
+                    || !(1..=MAX_DESKTOP_DIM).contains(&desktop_size.height)
+                {
+                    return Err(reason_err!(
+                        "ConnectionActivation::CapabilitiesExchange",
+                        "server desktop size {}x{} is outside the supported range",
+                        desktop_size.width,
+                        desktop_size.height
+                    ));
+                }


[skeptical] Native clients still abort on a valid maximum desktop size — low 🟡 — With the new bound, a conforming server can still announce 32766x32766 (~4.3 GB at 32bpp). Native clients (ironrdp-client, FFI) keep the infallible DecodedImage::new, which performs unchecked multiplication and vec![0; len], so a memory-constrained host still aborts on a fully valid announcement; only the out-of-spec/overflowing cases are eliminated. The PR body acknowledges this and defers a fallible DecodedImage::new to a follow-up, which is reasonable scope isolation, but the original failure mode is narrowed rather than closed for native clients and the follow-up should be tracked.

github-actions · 2026-10-07T15:45:19Z

+fn ensure_framebuffer_allocatable(width: u16, height: u16) -> anyhow::Result<()> {
+    let len = usize::from(width)
+        .checked_mul(usize::from(height))
+        .and_then(|pixels| pixels.checked_mul(usize::from(PixelFormat::RgbA32.bytes_per_pixel())))
+        .with_context(|| format!("framebuffer size overflows for a {width}x{height} desktop"))?;
+
+    Vec::<u8>::new()
+        .try_reserve_exact(len)
+        .with_context(|| format!("not enough memory for a {width}x{height} desktop framebuffer"))?;
+
+    Ok(())
+}


[code-compressor] ensure_framebuffer_allocatable can take connector::DesktopSize instead of two u16s — low 🟡 — Both call sites (session.rs:673-676 and :1081) pass width and height extracted from a connector::DesktopSize already in scope. Changing the signature to accept &connector::DesktopSize (a type already referenced in this file) removes field duplication at both call sites and ~3 lines with no behavior change. The checked_mul chain should be kept as-is: removing it would hard-code the connector's 1..=32766 invariant into ironrdp-web. Cosmetic, low-impact simplification.

Mathieu Morrissette (mmorrissette-devolutions) · 2026-10-07T17:38:20Z

fix for #2082

meanaverage (meanaverage) · 2026-10-07T18:21:44Z

Thanks—this blocks the original 65535×65535 trigger and materially improves the web path.

I verified one residual case at the current PR head: 32766×32766 remains accepted and produces a 4,294,443,024-byte RGBA32 allocation. A native DecodedImage::new probe under a 1 GiB address-space limit aborted with exit code 134:

memory allocation of 4294443024 bytes failed

Native consumers still reach the infallible vec![0; len] allocation, so the same remote process-abort class remains with an accepted desktop size. The web preflight also reserves and drops a temporary buffer before the separate infallible allocation.

I think this PR is valuable and can merge independently, but could issue #2082 remain open—or could a linked follow-up be created—for a fallible DecodedImage::try_new using checked arithmetic and fallible allocation, propagated through web and native consumers? A configurable framebuffer-byte budget would provide additional defense against protocol-valid but impractical dimensions.

This comment was marked as resolved.

Sign in to view

Mathieu Morrissette (mmorrissette-devolutions) force-pushed the fix/connector-bound-server-desktop-size branch from 43466ec to d0b0b57 Compare October 6, 2026 18:17

Mathieu Morrissette (mmorrissette-devolutions) marked this pull request as ready for review October 6, 2026 18:32

Copilot AI balanced review requested due to automatic review settings October 6, 2026 18:32

Copilot started reviewing on behalf of Mathieu Morrissette (mmorrissette-devolutions) October 6, 2026 18:32 View session

Mathieu Morrissette (mmorrissette-devolutions) deployed to llm-providers October 6, 2026 18:33 — with GitHub Actions Active

This comment was marked as resolved.

Sign in to view

Mathieu Morrissette (mmorrissette-devolutions) force-pushed the fix/connector-bound-server-desktop-size branch from d0b0b57 to e940cd5 Compare October 6, 2026 19:26

Mathieu Morrissette (mmorrissette-devolutions) deployed to llm-providers October 6, 2026 19:27 — with GitHub Actions Active

github-actions Bot added scope/web Affects the web/WASM ecosystem size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure and removed size/XS Size: up to 49 counted lines and 2 files labels Oct 6, 2026

Mathieu Morrissette (mmorrissette-devolutions) force-pushed the fix/connector-bound-server-desktop-size branch from e940cd5 to 547b6bf Compare October 6, 2026 19:46

Mathieu Morrissette (mmorrissette-devolutions) deployed to llm-providers October 6, 2026 19:48 — with GitHub Actions Active

This comment was marked as resolved.

Sign in to view

Mathieu Morrissette (mmorrissette-devolutions) force-pushed the fix/connector-bound-server-desktop-size branch from 547b6bf to 0d4e270 Compare October 6, 2026 20:34

Mathieu Morrissette (mmorrissette-devolutions) deployed to llm-providers October 6, 2026 20:35 — with GitHub Actions Active

chatgpt-codex-connector Bot reviewed Oct 6, 2026

View reviewed changes

Comment thread crates/ironrdp-web/src/session.rs Outdated

Mathieu Morrissette (mmorrissette-devolutions) force-pushed the fix/connector-bound-server-desktop-size branch from 0d4e270 to 95881d0 Compare October 7, 2026 11:55

Mathieu Morrissette (mmorrissette-devolutions) deployed to llm-providers October 7, 2026 11:56 — with GitHub Actions Active

Mathieu Morrissette (mmorrissette-devolutions) requested a review from Benoît Cortier (CBenoit) October 7, 2026 12:37

github-actions Bot added kind/protocol Affects RDP or related protocol behavior risk/medium Behavioral change that does not substantially alter a core public API labels Oct 7, 2026

github-actions Bot removed risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny automation-failed Exact-head automated classification or review failed or was unavailable labels Oct 7, 2026

github-actions Bot reviewed Oct 7, 2026

View reviewed changes

github-actions Bot added ai-reviewed/1 One automated review completed needs-author-action The pull request author is the current next actor labels Oct 7, 2026

Mathieu Morrissette (mmorrissette-devolutions) force-pushed the fix/connector-bound-server-desktop-size branch from 95881d0 to c4c14fb Compare October 7, 2026 15:17

Mathieu Morrissette (mmorrissette-devolutions) deployed to llm-providers October 7, 2026 15:21 — with GitHub Actions Active

github-actions Bot removed the needs-author-action The pull request author is the current next actor label Oct 7, 2026

github-actions Bot reviewed Oct 7, 2026

View reviewed changes

github-actions Bot added ai-reviewed/2 Two automated reviews completed needs-author-action The pull request author is the current next actor and removed ai-reviewed/1 One automated review completed labels Oct 7, 2026

Conversation

Mathieu Morrissette (mmorrissette-devolutions) commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests

Compatibility

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

Mathieu Morrissette (mmorrissette-devolutions) commented Oct 7, 2026

Uh oh!

meanaverage (meanaverage) commented Oct 7, 2026

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Mathieu Morrissette (mmorrissette-devolutions) commented Oct 6, 2026 •

edited

Loading

Benoît Cortier (CBenoit) commented Oct 7, 2026 •

edited

Loading