Skip to content

fix(connector): surface actual PDU type when an unexpected Share Control PDU arrives#1236

Merged
Benoît Cortier (CBenoit) merged 2 commits intoDevolutions:masterfrom
GlassOnTin:fix/surface-pdu-type-on-unexpected-share-control
Apr 29, 2026
Merged

fix(connector): surface actual PDU type when an unexpected Share Control PDU arrives#1236
Benoît Cortier (CBenoit) merged 2 commits intoDevolutions:masterfrom
GlassOnTin:fix/surface-pdu-type-on-unexpected-share-control

Conversation

@GlassOnTin
Copy link
Copy Markdown
Contributor

Closes part of #1232.

Why

Three error sites in the connector previously printed only the expected PDU type, dropping the actual type the server sent on the floor. That made interop bugs harder to diagnose than necessary.

Concrete case from #1232 (originally surfaced via Haven#109 on a GNOME Remote Desktop server): when a client connects to GRD without advertising EGFX, GRD's `freerdp_peer::Capabilities()` callback fails — confirmed via journalctl on the server:

```
gnome-remote-desktop-daemon: [RDP] Client did not advertise support for the Graphics Pipeline, closing connection
[ERROR] [rdp_peer_handle_state_demand_active]:
[CONNECTION_STATE_CAPABILITIES_EXCHANGE_DEMAND_ACTIVE] freerdp_peer::Capabilities() callback failed
```

GRD then sends a Server Deactivate All PDU instead of Server Demand Active. The connector saw `ServerDeactivateAll`, didn't match `ServerDemandActive`, and reported "unexpected Share Control Pdu (expected ServerDemandActive)" — with no hint that the server had sent Deactivate All (which itself is a strong signal the server gave up during caps validation, e.g. because of a missing capability).

What

Replaces three `general_err!` sites in the connector with `reason_err!` calls that include `pdu.as_short_name()` (the helper already exists on `ShareControlPdu`):

File Function Diagnosis context
`connection_activation.rs:125` `ConnectionActivation::CapabilitiesExchange` server's reply to `ConfirmActive`
`legacy.rs:152` `decode_share_data` wraps `decode_share_control` for callers expecting Data
`legacy.rs:213` `decode_io_channel` I/O channel dispatcher for the session loop

Now diagnoses look like:

unexpected Share Control PDU during capabilities exchange: got Server Deactivate All PDU (expected Server Demand Active PDU)

vs. the previous:

unexpected Share Control Pdu (expected ServerDemandActive)

which immediately points at GRD-style "I rejected your caps and deactivated the share" rather than leaving callers to guess from network traces.

Scope

  • No behavior change beyond the error string.
  • Existing matches on `ServerDemandActive` / `Data` / `ServerDeactivateAll` continue to dispatch the same way.
  • The wider EGFX-as-client work (so IronRDP-based clients can actually interop with GRD instead of just diagnosing the failure better) is a much larger separate effort — flagged in the issue thread.

Verification

  • `cargo build -p ironrdp-connector` clean.
  • `cargo clippy -p ironrdp-connector` clean (no new warnings; pre-existing duplicates persist).
  • `cargo test -p ironrdp-connector --lib` — 0 tests in the lib, no regressions.
  • No tests grep for the old error strings.

…rol PDU arrives

Three error sites in the connector previously printed only the
expected PDU type, dropping the actual type the server sent on the
floor. Made debugging interop bugs harder than it needed to be.

Specifically the GNOME Remote Desktop interop case (Devolutions#1232,
GlassHaven/Haven#109): when Haven connects to a GRD server, GRD's
freerdp_peer::Capabilities() callback fails because the client did
not advertise the Graphics Pipeline (EGFX). GRD then sends a Server
Deactivate All PDU instead of Server Demand Active. The connector
saw `ServerDeactivateAll`, didn't match `ServerDemandActive`, and
reported "unexpected Share Control Pdu (expected ServerDemandActive)"
— with no hint that the server had sent Deactivate All (which itself
is a strong signal the server gave up during caps validation).

This commit replaces the three offending `general_err!` sites with
`reason_err!` calls that include `pdu.as_short_name()` (the helper
already exists on `ShareControlPdu`):

  connection_activation.rs:125  ConnectionActivation::CapabilitiesExchange
  legacy.rs:152                 decode_share_data
  legacy.rs:213                 decode_io_channel

Now diagnoses look like:

  unexpected Share Control PDU during capabilities exchange:
    got Server Deactivate All PDU
    (expected Server Demand Active PDU)

which immediately points at GRD-style "I rejected your caps and
deactivated the share" rather than leaving callers to guess.

No behavior change beyond the error string. Existing call sites that
match `ServerDemandActive` / `Data` / `ServerDeactivateAll` continue
to dispatch the same way.
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@CBenoit Benoît Cortier (CBenoit) enabled auto-merge (squash) April 28, 2026 14:31
auto-merge was automatically disabled April 28, 2026 14:40

Head branch was pushed to by a user without write access

@CBenoit Benoît Cortier (CBenoit) enabled auto-merge (squash) April 29, 2026 11:18
@CBenoit Benoît Cortier (CBenoit) merged commit 78effb3 into Devolutions:master Apr 29, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants