fix(vmm): cap volume_device_name at MAX_VOLUMES (closes #175)#176
Merged
Conversation
The doc on volume_device_name said "up to 23 → vdy", but the function
itself had no actual cap. Indices 25..153 produced garbage device names
("vd{", "vd|", "vd}", "vd~", …) because (b'b' + index as u8) silently
walks past 'z' into the ASCII punctuation range. The host-side cmdline
append happily included these names in `forkd.mounts=`; the guest then
silently failed to mount the volume since /dev/vd{ doesn't exist.
This change:
- Adds `pub const MAX_VOLUMES: usize = 25;` matching the real virtio-blk
/dev/vdb..vdz range (/dev/vda is the rootfs).
- Returns `Result<String>` from `volume_device_name` and bails for
`index >= MAX_VOLUMES` with a message naming the cap.
- Propagates the error through `BootConfig::with_volume`, which now
returns `Result<Self>`. The sole non-test caller in forkd-cli wraps
it with `with_context` so the CLI surface gets a contextual error
rather than a panic.
- Adds two regression tests:
- `volume_device_name_rejects_past_cap` — indices MAX_VOLUMES,
MAX_VOLUMES+1, 100, 153.
- `boot_config_with_volume_errors_past_cap` — fills to MAX_VOLUMES,
asserts the 26th attachment fails instead of producing "vd{".
- Updates the existing `volume_device_name_progression` test to also
assert the last valid index (MAX_VOLUMES - 1) → "vdz".
Note: this is a small API break — `with_volume` was previously
infallible and is now `Result`. Only one in-tree caller exists
(crates/forkd-cli/src/main.rs); it's updated in this PR.
Reported by @andraiming in #175. Closes #175.
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.
Closes #175. Thanks to @andraiming for the precise repro and source-cite.
Root cause
volume_device_name's doc said "up to 23 → vdy" but the function itself had no actual cap. Indices 25..153 produced garbage device names likevd{,vd|,vd},vd~because(b'b' + index as u8) as charwalks pastzinto ASCII punctuation. The host happily inserted those names into theforkd.mounts=kernel cmdline, and the guest's/forkd-init.shthen silently failed to mount the volume (no/dev/vd{device exists).The fix
pub const MAX_VOLUMES: usize = 25;— matches the real virtio-blk/dev/vdb..vdzrange (/dev/vdais the rootfs).volume_device_namenow returnsResult<String>and bails forindex >= MAX_VOLUMESwith a message naming the cap.BootConfig::with_volumepropagates the error and now returnsResult<Self>.crates/forkd-cli/src/main.rsis updated with.with_context(...)so users see a contextual CLI error instead of a panic.Tests
Existing
volume_device_name_progressionextended to also assert the last valid index (MAX_VOLUMES - 1 → "vdz"). Two new regression tests added:volume_device_name_rejects_past_cap— exercisesMAX_VOLUMES,MAX_VOLUMES + 1,100,153(the historical worst case where addition wraps into the next byte per @andraiming's analysis).boot_config_with_volume_errors_past_cap— fillswith_volumeto capacity, asserts the 26th attachment fails rather than silently producingvd{.Local results on the dev box (
cargo test -p forkd-vmm --lib volume):cargo check -p forkd-clialso passes cleanly with the new?propagation inwith_volume.API compatibility
This is a small API break:
BootConfig::with_volumewas previously infallible and is nowResult<Self>. Only one in-tree caller exists (crates/forkd-cli/src/main.rs) and it's updated in this PR. Out-of-tree callers building against the forkd-vmm crate will need a single?(or.expect(...)) added.