fix(core): refuse a byte size that does not fit instead of wrapping it to zero - #190
Merged
Merged
Conversation
…t to zero `ByteSize::parse` reads the digits, matches the suffix and then multiplies, and that multiply was unchecked. In a debug build it panics; in a release build it wraps, which is the case that matters, because it wraps into a number the caller never wrote and nothing reports it. `memory = "17179869184G"` is exactly 2^64 bytes. It wrapped to 0 and installed a ceiling of zero, and a memory ceiling of zero is not inert: the supervisor registers the handler on `is_some()`, so the guest is SIGKILLed on its first anonymous mmap while `/proc/meminfo` inside the sandbox reports the sandbox unlimited. The two readings cannot both stand, and neither of them names the setting that caused it. The suffix now resolves to a scale and the multiply is `checked_mul`, so a value that does not fit is `byte size out of range: <value>`, naming what was written. The largest value each suffix can carry still parses, so this refuses what does not fit rather than trimming the usable range. `ByteSize::kib`, `mib` and `gib` keep their unchecked multiplies. They take a number the caller already holds rather than text the caller wrote, so the question they answer is different, and changing their signatures would be a breaking change to a public type for a case no parse path reaches.
Contributor
|
Thanks for the quick fix! |
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.
Small, self-contained, and safe for the release you are about to cut.
The bug
ByteSize::parsereads the digits, matches the suffix and then multiplies,and that multiply is unchecked. A debug build panics; a release build wraps,
which is the case that matters, because it wraps into a number the caller
never wrote and nothing reports it.
memory = "17179869184G"is exactly 2^64 bytes. It wraps to 0 and installs aceiling of zero. That ceiling is not inert: the supervisor registers the
memory handler on
is_some(), so the guest is SIGKILLed on its firstanonymous mmap, while
/proc/meminfoinside the sandbox reports the sandboxunlimited. Both readings cannot stand, and neither names the setting that
caused it.
Reachable from every surface that takes a size:
--max-memory,--max-disk,[limits].memory,[limits].disk, and the builder.The fix
The suffix resolves to a scale and the multiply becomes
checked_mul, so avalue that does not fit is reported as
byte size out of range: <value>,naming what was written. The largest value each suffix can carry still parses,
so this refuses what does not fit rather than trimming the usable range.
ByteSize::kib,mibandgibkeep their unchecked multiplies on purpose.They take a number the caller already holds rather than text the caller wrote,
so the question they answer is different, and changing their signatures would
be a breaking change to a public type for a case no parse path reaches.
Testing
test_bytesize_that_does_not_fit_is_an_error_not_a_ceiling_of_zerocovers theoverflowing value for each of the three suffixes and pins the boundary that
must still parse. Verified it fails against the previous behaviour: with the
multiply put back, the test reports that
17179869184Gwas accepted.cargo test -p sandlock-core --lib: 683 pass.cargo test -p sandlock-core --test integration: 395 pass.Where this comes from
This is one commit lifted out of #185, which you suggested deferring to the
next release. I agree with deferring that one, but this defect is independent
of everything else in it and costs 40 lines, so it seemed worth offering on
its own rather than holding a silent SIGKILL back for a release cycle. Take it
or leave it for the same batch, either is fine.