fix(connector,session): propagate negotiated share_id to all outgoing ShareDataPdu

[Greg Lamberson (glamberson)]

Summary

Fixes the share_id always being sent as 0 in all outgoing ShareDataPdu messages.
This is one of two protocol conformance bugs identified by Vladyslav Nikonov (@pacmancoder) in
#447 (comment) (May 2024),
and is the most likely cause of the progressive frame delivery degradation after resize
originally reported by Isaiah Becker-Mayer (@ibeckermayer) in #447.

Background

Per MS-RDPBCGR 2.2.8.1.1.1.1 (Share Control Header), the shareId field identifies
the shared session between client and server. The server assigns this value in the
Server Demand Active PDU, and the client must echo it in all subsequent Share Control
and Share Data PDUs.

During the initial connection, IronRDP correctly decodes the share_id from the Server
Demand Active and uses it in the Client Confirm Active response
(ConnectionActivationSequence::step() at connection_activation.rs:171). However,
the value was never stored beyond that point. Every subsequent outgoing ShareDataPdu
used a hardcoded share_id: 0.

This matters most after a Deactivation-Reactivation Sequence (MS-RDPBCGR 1.3.1.3),
which occurs when the server processes a DISPLAYCONTROL_MONITOR_LAYOUT resize request.
The server sends a new Deactivate All followed by a new Demand Active with a potentially
different share_id. Since the client was always sending 0 regardless, the mismatch
between the server's expected share_id and the client's actual share_id could cause
the server to ignore or deprioritize frame acknowledges, producing exactly the
"frames slow down after resize" symptom that Isaiah Becker-Mayer (@ibeckermayer) reported.

I was able to reproduce this degradation pattern testing with encode_resize() from
ActiveStage against GNOME/Mutter (Fedora 43) and Hyprland (Arch), both using the
EGFX V10 AVC420 codec path. See #447 (comment) for details.

What this PR does

Captures the share_id from the Server Demand Active PDU and threads it through every
code path that emits outgoing ShareDataPdu messages:

ironrdp-connector:

• ConnectionActivationState::ConnectionFinalization and ::Finalized now carry share_id
• ConnectionFinalizationSequence stores share_id and uses it for all four finalization
  PDUs (Synchronize, ControlCooperate, RequestControl, FontList) instead of 0
• ConnectionResult exposes share_id so downstream consumers have access to the
  negotiated value

ironrdp-session:

• x224::Processor stores share_id and uses it in encode_static() (input events,
  shutdown request) instead of 0
• FrameMarkerProcessor stores share_id and uses it in frame acknowledge PDUs instead
  of 0. This is the most performance-critical fix: frame acks are the server's primary
  flow control signal for EGFX frame delivery.
• ProcessorBuilder accepts share_id so it can be set both at initial connection and
  after reactivation

Application-level reactivation handlers:

• ironrdp-client/src/rdp.rs, ironrdp-web/src/session.rs, and
  ironrdp-testsuite-extra/tests/mod.rs all destructure the new share_id field from
  ConnectionActivationState::Finalized and pass it through when rebuilding the
  FastPathProcessor after reactivation

FFI bindings:

• ConnectionResult::get_share_id() added
• ConnectionActivationStateFinalized carries and exposes share_id
• ActiveStage::set_fastpath_processor() accepts share_id parameter

Affected callsites

Seven callsites were sending hardcoded share_id: 0:

Location	PDU	Fix
connection_finalization.rs:101	Synchronize	self.share_id
connection_finalization.rs:118	ControlCooperate	self.share_id
connection_finalization.rs:135	RequestControl	self.share_id
connection_finalization.rs:145	FontList	self.share_id
x224/mod.rs:200	Input events, ShutdownRequest	self.share_id
fast_path.rs:630	FrameAcknowledge	self.share_id
connection_activation.rs:171	ClientConfirmActive	(already correct)

Verification

• cargo xtask check fmt -v passes
• cargo xtask check lints -v passes
• cargo xtask check tests -v passes (all existing tests; wire format is unchanged,
  only the value placed in the share_id field changes)

Related

• Devilish resize bug #447 (original issue: resize performance degradation)
• A follow-up PR will fix the ShareDataHeader::uncompressedLength calculation
  (the second protocol bug Vladyslav Nikonov (@pacmancoder) identified in the same comment)
• A separate follow-up will revert max_unacknowledged_frame_count from 20 back to 2
  (the FIXME(#447) workaround in connection_activation.rs) once this fix is validated

[Copilot]

Pull request overview

This PR fixes a protocol conformance bug where share_id was always sent as 0 in all outgoing ShareDataPdu messages. The correct behavior per MS-RDPBCGR is to echo back the share_id assigned by the server in its Demand Active PDU. The most performance-critical fix is the frame acknowledge PDU path, since mismatched share_id values in frame ACKs could cause servers to delay or ignore them, leading to the "frames slow down after resize" regression reported in issue #447.

Changes:

• ironrdp-connector: ConnectionFinalizationSequence and ConnectionActivationState now carry and propagate the negotiated share_id; ConnectionResult exposes it to downstream consumers.
• ironrdp-session: x224::Processor and FrameMarkerProcessor store the share_id and use it in encode_static() and frame acknowledge PDUs respectively; ProcessorBuilder accepts share_id.
• Application-level handlers and FFI: all reactivation handlers in ironrdp-client, ironrdp-web, the test suite, and the FFI layer now extract and pass through the new share_id from ConnectionActivationState::Finalized.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/ironrdp-connector/src/connection_activation.rs	Captures share_id from Server Demand Active; threads it through ConnectionFinalization and Finalized states
crates/ironrdp-connector/src/connection_finalization.rs	Stores share_id in ConnectionFinalizationSequence; uses it for all 4 finalization PDUs
crates/ironrdp-connector/src/connection.rs	Adds share_id field to ConnectionResult; destructures it from ConnectionActivationState::Finalized
crates/ironrdp-session/src/x224/mod.rs	Stores share_id in x224::Processor; uses it in encode_static()
crates/ironrdp-session/src/fast_path.rs	Adds share_id to ProcessorBuilder and FrameMarkerProcessor; uses it in frame acknowledge PDUs
crates/ironrdp-session/src/active_stage.rs	Passes connection_result.share_id to both x224::Processor and ProcessorBuilder
crates/ironrdp-client/src/rdp.rs	Destructures and threads share_id through the reactivation handler
crates/ironrdp-web/src/session.rs	Destructures and threads share_id through the reactivation handler
crates/ironrdp-testsuite-extra/tests/mod.rs	Destructures and threads share_id through the reactivation test
ffi/src/connector/activation.rs	Exposes share_id in ConnectionActivationStateFinalized; correctly ignores it for intermediate ConnectionFinalization state
ffi/src/connector/result.rs	Exposes get_share_id() method on FFI ConnectionResult
ffi/src/session/mod.rs	Accepts share_id parameter in set_fastpath_processor()
ffi/dotnet/Devolutions.IronRdp.AvaloniaExample/MainWindow.axaml.cs	Passes shareId to SetFastpathProcessor() in reactivation handler

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Benoît Cortier (CBenoit)]

Sounds good! Maybe double check the Copilot comment which sounds suspicious, but otherwise looking good to me!

[Greg Lamberson (glamberson)]

Sounds good! Maybe double check the Copilot comment which sounds suspicious, but otherwise looking good to me!

Hi,
I checked the Copilot comment and it's valid.

During Deactivation-Reactivation, set_fastpath_processor() replaces the fast_path processor with a new one carrying the updated share_id. But x224_processor is never updated. There's no set_x224_share_id() or equivalent. So after reactivation, encode_static() (used for ShutdownRequest, slow-path input, etc.) would send PDUs with the stale share_id.

In practice most Windows servers reuse the same share_id across reactivation, so this rarely surfaces, but the spec doesn't guarantee it.

The fix is straightforward. I'll add a method to update the x224 processor's share_id and call it alongside set_fastpath_processor() in the reactivation handlers.

Thanks,
Greg

[Benoît Cortier (CBenoit)]

LGTM!