Devolutions / Devolutions/IronRDP

Optional EGFX payload dump for offline triage of decode/encode failures

Open
#1,241 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
3.2k
Forks
275
Avg merge
1d 11h
Merged PRs (30d)
189

Description

## Summary

Propose adding an opt-in payload-dump capability to `ironrdp-egfx` (and symmetrically to `ironrdp-server`) triggered by an `IRONRDP_EGFX_DUMP_DIR` env var. When the env var is unset, zero behavior change and a single `env::var_os()` cold-path check on the error path only.

## Why

EGFX decode failures against modern Windows servers are notoriously hard to diagnose without the failing bytes on disk. The PDU layer hands `bitmap_data` to the codec dispatch and a `Result<()>` comes back. If the codec rejects, the only artifact is a `tracing::warn!` line. Reproducing requires re-running the entire RDP session against the same server in the same state, which is often unavailable.

The pattern is widely used in similar projects:
- FreeRDP has codec-specific dumps via its WLOG appender plus `winpr/tools/rdp-decoder`.
- A downstream IronRDP consumer (Haven, github.com/GlassHaven/Haven) ships an `EGFX_DUMP_DIR` env var doing exactly this.

When triaging the RDPGFX_RECT16 off-by-one in #1238 (caught by Haven's dump-equipped decoder), having the failing bytes on disk would have shaved hours off reproduction.

## Proposed shape

`crates/ironrdp-egfx/src/client.rs`: at each codec dispatch error path (`decode_avc420`, `decode_clearcodec`, `decode_avc444`, `handle_uncompressed`, future `decode_progressive`), check `IRONRDP_EGFX_DUMP_DIR`. If set, write:
- `{millis}_{codec}_surface{id}.bin` containing the raw `bitmap_data` bytes
- `{millis}_{codec}_surface{id}.json` containing `{rect, codec, surface_id, error}` (hand-rolled, no `serde_json` dep added)

`crates/ironrdp-server/src/encoder/mod.rs`: parallel hook on encode error paths. Dumps the source pixel buffer and any partial output. The server-side feature is rarer in practice (we control the bytes we produce) but symmetric for completeness.

Naming: `IRONRDP_EGFX_DUMP_DIR` matches the existing `IRONRDP_LOG_PATH` / `IRONRDP_LOG` env conventions. Cost when unset: one `env::var_os()` call on the error path only, no impact on success paths or builds.

## Questions before I PR this

1. Is this kind of runtime payload-capture welcome in `ironrdp-egfx`, or would you prefer it live in a separate `ironrdp-egfx-debug` crate behind a feature flag?
2. Server-side encode-error dump: include in the same PR or follow-up? My read is that server-side encode errors are rare enough that the value is mostly symmetric / completeness; the high-value case is client-side decode errors.
3. Naming: `IRONRDP_EGFX_DUMP_DIR` ok, or prefer something different (e.g., `IRONRDP_DUMP_DIR` if you imagine extending to other codecs / channels)?

Happy to PR this once the shape is endorsed.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.