Skip to content

fix(ffi): dispose CredSSP generator before reuse - #2160

Merged
irvingouj@Devolutions (irvingoujAtDevolution) merged 1 commit into
fix/ffi-fill-larger-bufferfrom
fix/ffi-credssp-generator-dispose
Oct 20, 2026
Merged

irvingouj@Devolutions (irvingoujAtDevolution) merged 1 commit into
fix/ffi-fill-larger-bufferfrom
fix/ffi-credssp-generator-dispose

Conversation

@irvingoujAtDevolution

@irvingoujAtDevolution irvingouj@Devolutions (irvingoujAtDevolution) commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

ProcessTsRequest returns a generator that holds an exclusive borrow of the CredSSP sequence. The CredSSP loop left it alive and then called HandleProcessResult on the same sequence, so two &mut borrows of it were live at once.

Dispose the generator as soon as it has produced the client state.

Co-Authored-By: Claude Opus 5.5 (1M context) noreply@anthropic.com

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The scoped disposal resolves the overlapping borrow while preserving the independently owned client state.

No open issues 🎉

What changed in this PR

Fixes the .NET CredSSP loop by disposing its generator before reusing the sequence.

Changes:

  • Scopes generator disposal around client-state resolution.
  • Releases the exclusive borrow before calling HandleProcessResult.
File Description
ffi/​dotnet/​Devolutions.IronRdp/​src/​ConnectionHelpers.cs Adds deterministic generator disposal before sequence reuse.

Review effort: Balanced

@github-actions github-actions Bot added risk/medium Behavioral change that does not substantially alter a core public API scope/ffi Affects native or .NET bindings size/XS Size: up to 49 counted lines and 2 files triage/overlap Possible overlap with another pull request; advisory only labels Oct 20, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR wraps the CredSSP generator in a block-scoped using in PerformCredsspSteps so the generator box (which holds an exclusive &mut CredsspSequence across the FFI boundary) is destroyed before credsspSequence.HandleProcessResult takes a second mutable borrow. Independently verified: get_client_state_if_completed(&self) clones the ClientState out (ffi/src/credssp/network.rs:62), so disposing before HandleProcessResult is safe and covers ResolveGenerator exception paths. The fix is correct and load-bearing (a using-var form would keep the generator alive across HandleProcessResult). The single valid skeptical candidate is a low-severity, pre-existing hygiene note that the same loop still abandons sibling FFI objects (TsRequest replacement at line 91, GeneratorState boxes in Connection.ResolveGenerator) to nondeterministic finalizers; it holds no borrow of the sequence, so no aliasing recurs, and it does not block the PR.

  1. [skeptical] Generator disposal fixes the aliasing, but the same loop still abandons other Rust-owned FFI objects to finalizers — low 🟡 — ffi/dotnet/Devolutions.IronRdp/src/ConnectionHelpers.cs
    The fix deterministically releases the CredsspProcessGenerator box, which uniquely borrows &mut CredsspSequence, so the stated aliasing UB is resolved (get_client_state_if_completed(&self) clones the ClientState out, so disposal cannot invalidate it, and using also covers ResolveGenerator exception paths). However, in the same loop each iteration replaces tsRequest (tsRequest = decoded, line 91) without disposing the previous TsRequest box, and Connection.ResolveGenerator (Connection.cs:61,88) allocates a new GeneratorState box per Start/Resume without disposing any; all rely on nondeterministic diplomat finalizers. These hold no borrow of the CredsspSequence, so no aliasing recurs, but ownership handling in this method becomes inconsistent: one FFI object gets explicit using while siblings get finalization. Pre-existing and non-regressing; it does not block this PR and should be addressed in a follow-up or the PR should record that only the generator uniquely borrows the sequence and thus warrants explicit disposal.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

@github-actions 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 20, 2026
@github-actions github-actions Bot added risk/low Self-contained change with no cross-crate behavioral effect and removed needs-author-action The pull request author is the current next actor risk/medium Behavioral change that does not substantially alter a core public API labels Oct 20, 2026
`ProcessTsRequest` returns a generator that holds an exclusive borrow
of the CredSSP sequence. The CredSSP loop left it alive and then called
`HandleProcessResult` on the same sequence, so two `&mut` borrows of
it were live at once.

Dispose the generator as soon as it has produced the client state.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added risk/medium Behavioral change that does not substantially alter a core public API and removed risk/low Self-contained change with no cross-crate behavioral effect triage/overlap Possible overlap with another pull request; advisory only labels Oct 20, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 PR 2160 wraps the CredsspProcessGenerator returned by ProcessTsRequest in a scoped using block in ConnectionHelpers.PerformCredsspSteps so it is disposed before credsspSequence.HandleProcessResult is invoked on the same sequence. Independently verified: CredsspProcessGenerator is a Diplomat IDisposable freeing a raw Rust allocation; ProcessTsRequest returns it holding an exclusive borrow of the sequence; Connection.ResolveGenerator consumes the generator to completion and returns an independently owned ClientState, so disposal after resolution (including on exception paths via the using block) is safe. The scoped form is the minimal correct construct - a using-declaration would dispose only at end of loop iteration, after HandleProcessResult, reintroducing the aliasing. Single call site, no scope creep. No valid specialist candidates and no independently discovered issues; findings and candidate_dispositions are empty.

@github-actions github-actions Bot removed the ai-reviewed/1 One automated review completed label Oct 20, 2026
@github-actions github-actions Bot added ai-reviewed/2 Two automated reviews completed needs-review A human reviewer is the current next actor labels Oct 20, 2026
@irvingoujAtDevolution
irvingouj@Devolutions (irvingoujAtDevolution) merged commit 3a3c27d into master Oct 20, 2026
44 checks passed
@irvingoujAtDevolution
irvingouj@Devolutions (irvingoujAtDevolution) deleted the fix/ffi-credssp-generator-dispose branch October 20, 2026 21:35
@github-actions github-actions Bot removed the needs-review A human reviewer is the current next actor label Oct 20, 2026

This branch was previously deployed

1 inactive deployment
llm-providers — 869e3d05 Deployed Oct 20, 2026 by irvingoujAtDevolution via Classify pull request #4397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/2 Two automated reviews completed risk/medium Behavioral change that does not substantially alter a core public API scope/ffi Affects native or .NET bindings size/XS Size: up to 49 counted lines and 2 files

Development

Successfully merging this pull request may close these issues.

3 participants