Repository navigation
nvx-rs: decode the EXEC header once for both payload decoders - #603
Merged
Merged
Conversation
decode_exec_payload, the text decoder that the fake OpenVMM uses, and decode_guest_exec_payload, the byte decoder that the guest agent uses, each carried the same 42 lines that read and check the fixed fields of an EXEC payload: the timeout, the number of arguments, the extension, and an extended payload's flags, environment count, and working-directory length. The copies differed only in the constant that bounds the working directory, MAX_ARGUMENT_BYTES in the host decoder and MAX_GUEST_CWD_BYTES in the guest decoder. The text readers, prefixed_text and text_at, also repeated the length, truncation, and NUL checks of the byte readers, prefixed_bytes and bytes_at. - decode_exec_header reads and checks the fixed fields once and returns them as an ExecHeader, which each decoder destructures. Each decoder passes its own working-directory bound, so the two constants stay separate. - prefixed_text and text_at read their bytes with prefixed_bytes and bytes_at, then require UTF-8 through utf8_text. What each decoder accepts, its error messages, and the order of its checks are unchanged. The byte readers advance the offset with checked additions where the text readers used plain ones, but each addition follows a successful bounds check, so neither can fail or overflow. The protocol tests pin both decoders' acceptances and refusals and the round trip through the guest decoder. A throwaway differential test compared the old and new decoders on every payload of up to 3 bytes and on 6 million structured and random payloads: their results, messages included, were identical. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused refactor preserves validation order, limits, and error behavior, with no unresolved findings.
No open issues 🎉
What changed in this PR
Refactors EXEC payload decoding to share validation logic between the host and guest decoders without changing public APIs or accepted payloads.
Changes:
- Extracts shared header validation while preserving each decoder’s working-directory limit.
- Builds text readers on byte readers and centralizes UTF-8 conversion.
| File | Description |
|---|---|
nvx-rs/src/protocol/exec.rs |
Consolidates header and field parsing while preserving validation behavior. |
Review effort: Balanced
This was referenced Oct 19, 2026
This was referenced Oct 20, 2026
This was referenced Oct 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
decode_exec_payload, the UTF-8 decoder that the fake OpenVMM uses, anddecode_guest_exec_payload, the byte decoder that the guest agent uses, each carried the same 42 lines that read and check the fixed fields of anEXECpayload. The text readersprefixed_textandtext_atalso repeated the length, truncation, and NUL checks of the byte readersprefixed_bytesandbytes_at. This change decodes the header in one private function and builds the text readers on the byte readers. The wire header's checks now live in one place, so the host and guest decoders cannot drift apart when a flag or limit changes.This is the first
nvx-rs-quality/run, and no earlier pull request in the series had feedback to follow. It picked the largest duplicate block innvx-rs/srcandnvx-rs/testsin a file that no open pull request todevchanges.Changes
All changes are in
nvx-rs/src/protocol/exec.rs: 68 insertions and 73 deletions. No public item, test, fixture, or other file changes.ExecHeaderanddecode_exec_header(payload, max_cwd)read and check the timeout, the number of arguments, the extension, and an extended payload's flags, environment count, and working-directory length. Two decoders now share this code:decode_exec_payloadpassesMAX_ARGUMENT_BYTESas the working-directory bound, as it checked before.decode_guest_exec_payloadpassesMAX_GUEST_CWD_BYTES, as it checked before.prefixed_textnow reads withprefixed_bytes, andtext_atreads withbytes_at. Both then convert through the newutf8_text, which holds theexec {what} must be valid UTF-8message once.Behavior
Each decoder accepts and refuses the same payloads as before, with the same messages and checks in the same order. The header code moved as it was, except that it takes its working-directory bound as an argument and returns its fields as a struct. The two bound constants are both 4096 today, and they stay separate.
The byte readers advance
offsetwith checked additions, where the text readers used+=. Each addition follows a successful bounds check, so neither can fail. On a UTF-8 error, a text reader now leavesoffsetpast the field. Every caller returns that error at once and never readsoffsetagain.I wrote a throwaway differential test, which is not committed. It compiled the original and new
exec.rsside by side and compared theDebugform of both decoders' results, error messages included, on three sets of payloads:All results were identical. The inputs reached all 35 distinct error messages of the two decoders, and each decoder accepted tens of thousands of payloads.
Follow-ups
decode_guest_exec_payloadsays that it differs fromdecode_exec_payload"at the C guest's 4096-byte working-directory boundary". Both decoders accept a 4096-byte working directory, though, becauseMAX_GUEST_CWD_BYTESisMAX_ARGUMENT_BYTES. Either the comment is stale, or the host decoder was meant to useMAX_CWD_BYTES(4095). Either fix changes documented behavior or acceptance, so this change leaves it alone.protocol/maps.rs,decode_maps_payloadanddecode_guest_maps_payloadrepeat their header and entry loop in the same way. The guest copy also refuses more thanMAX_HOST_MAPPINGSentries up front and uses checked offset arithmetic, while the host copy does neither. Sharing that code needs a decision on whether the difference is intended.EXECdecoders, which read the arguments, working directory, and environment, still mirror each other. Their environment messages andinherit_default_envrules differ on purpose, so this change keeps them apart.Validation
BASE:
7e96632b3d6da4200b09b4102296cd0b11cdeb83, which wasorigin/devwhen the run started. Before this pull request was opened, no laterdevcommit and no open pull request todevchangednvx-rs/src/protocol/exec.rs.TREE:
1568601977449421a4dbb2b629f589d9dda1d46e, the tree of this commit. Every host applied the patch to BASE and checked thatgit write-treeequals TREE.Release on every host:
v0.1.0-dev.3e740c0e1c9a. Its OpenVMM,5ab8a84621524f1777b2cb5e65a70dab42c8b453, is the pinned revision.Guest artifacts were rebuilt from TREE on prometheus30, with these steps:
build-guest-binaries --output;test-guest-binaries --binaries, where 34 tests passed;build-initramfswithNVX_GUEST_BINARIES.initramfs.cpio.gz(SHA-25683d4218cb082db3038a659075cae8d4634086470c6a14f569077e65ad98f3d22), its packages manifest, and its provenance went to the other two hosts, where the SHA-256 digests matched.Local checks on Windows with Rust 1.95.0:
-D warnings, for all targets and features and for the library without default features;cargo testwith all features (181 library tests, 6 fake-OpenVMM tests, and 1 doctest) and with the default features (178 and 1);-D warnings;aarch64-apple-darwin) checks, for bothnvx-rsandaci_edge_sandboxes;test-aci-edge-sandboxestest-microvm, default suitecheck-guest-binaries(277 + 34 musl tests), artifact bundling, and agent bundling; the macOS checks ran locallyLeft to CI:
test-openvmm-unitandtest-openvmm;smp-lapic,managed-exec-config, andsandbox-lifecycle;