Skip to content

refactor(async,blocking)!: make Framed::read_exact crate-private#1247

Merged
Benoît Cortier (CBenoit) merged 1 commit into
Devolutions:masterfrom
lamco-admin:refactor/framed-read-exact-private
May 14, 2026
Merged

refactor(async,blocking)!: make Framed::read_exact crate-private#1247
Benoît Cortier (CBenoit) merged 1 commit into
Devolutions:masterfrom
lamco-admin:refactor/framed-read-exact-private

Conversation

@glamberson
Copy link
Copy Markdown
Contributor

Summary

Closes #1226.

Framed holds an invariant that buf starts at the first byte of a PDU message. read_pdu and read_by_hint maintain that invariant. Framed::read_exact also maintains it for its own callers, but exposing it as pub lets external callers leave buf misaligned by passing an arbitrary length, which then breaks subsequent read_pdu calls.

Both ironrdp_async::framed::Framed and ironrdp_blocking::framed::Framed have the same shape and the same exposure. Audit shows no external consumers across the workspace; the four call sites are all internal:

Crate Call site
ironrdp-async framed.rs:145 (read_pdu)
ironrdp-async framed.rs:174 (read_by_hint)
ironrdp-blocking framed.rs:74 (read_pdu)
ironrdp-blocking framed.rs:96 (read_by_hint)

Both visibility changes use pub(crate).

Also drops the #[expect(clippy::missing_panics_doc)] annotation that was placed on the inline expect("length > self.buf.len()") because the lint only fires on public APIs; with the methods now pub(crate), the expectation is unfulfilled and clippy fails the build.

Breaking change

Marked with ! in the conventional commit. Affects any out-of-workspace consumer that called Framed::read_exact directly. The audit was workspace-only; if someone has a private consumer that relied on this, the migration is to use read_pdu (when reading a structured PDU) or read_by_hint (when the length comes from a header peek) — both maintain the buffer invariant correctly.

Pre-1.0 (ironrdp-async 0.8, ironrdp-blocking 0.8); per Cargo SemVer a 0.8 → 0.9 minor bump covers this.

Test plan

  • cargo xtask check fmt -v clean
  • cargo xtask check lints -v clean
  • cargo xtask check tests -v passes
  • cargo build --workspace --all-targets clean

Closes Devolutions#1226. Framed holds an invariant that buf starts at the first
byte of a PDU message. read_pdu and read_by_hint maintain that
invariant. Framed::read_exact also maintains it for its own callers,
but exposing it as pub lets external callers leave buf misaligned by
passing an arbitrary length, which then breaks subsequent read_pdu
calls.

Both ironrdp-async::framed::Framed and ironrdp-blocking::framed::Framed
have the same shape and the same exposure. Audit shows no external
consumers across the workspace (the four call sites are all internal:
two each in async/framed.rs and blocking/framed.rs, calling self.read_exact
inside read_pdu / read_by_hint).

Drop the now-unused missing_panics_doc lint expectation since the
function is no longer part of the documented public API.

This is a breaking change to the public API of ironrdp-async (0.8) and
ironrdp-blocking (0.8). Pre-1.0; per Cargo SemVer the next minor bump
in either crate covers this.
@glamberson
Copy link
Copy Markdown
Contributor Author

copilot-pull-request-reviewer please review

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.

Easy win!

@CBenoit Benoît Cortier (CBenoit) merged commit d02d24a into Devolutions:master May 14, 2026
13 checks passed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Restricts Framed::read_exact to crate-private visibility in both ironrdp-async and ironrdp-blocking to protect the internal buffer alignment invariant maintained by read_pdu/read_by_hint. Also removes now-unfulfilled #[expect(clippy::missing_panics_doc)] annotations since the lint no longer fires on non-public methods.

Changes:

  • Change pub fn read_exact to pub(crate) fn read_exact in ironrdp-async and ironrdp-blocking.
  • Remove #[expect(clippy::missing_panics_doc, ...)] annotations that would otherwise become unfulfilled expectations.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/ironrdp-async/src/framed.rs Restrict async read_exact visibility and drop unfulfilled clippy expectation.
crates/ironrdp-blocking/src/framed.rs Restrict blocking read_exact visibility and drop unfulfilled clippy expectation.

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

@glamberson Greg Lamberson (glamberson) deleted the refactor/framed-read-exact-private branch May 15, 2026 18:00
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.

Framed::read_exact should not be pub

4 participants