Fix u32 overflow in set_path on oversized paths - #1152
Open
pacocartones wants to merge 1 commit into
Open
Conversation
Url stores query_start and fragment_start as u32 offsets into the serialization. restore_after_path rebases them onto the new end of the path with plain u32 arithmetic, so a path whose percent-encoded form pushes the serialization to the u32 limit makes that arithmetic overflow: it panics with overflow checks on, and wraps without them, leaving query() and fragment() pointing at arbitrary parts of the string. set_path now notices when the result cannot fit in those offsets and leaves the URL untouched instead. The check only costs anything for inputs large enough that overflow is possible at all: percent-encoding grows the input by at most 3x, so for every ordinary path the upper bound is known to fit up front and nothing extra is done. Fixes servo#1106
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.
Fixes #1106.
The bug
Urlkeepsquery_startandfragment_startasu32offsets into theserialization.
set_pathreparses the path and then callsrestore_after_path, which rebases those offsets onto the new end of the pathwith plain
u32arithmetic:Nothing checks that the result still fits, so a path whose percent-encoded form
pushes the serialization up against
u32::MAXoverflows that addition. With therepro from the issue on
main(00a6ce5):with overflow checks on:
panicked at url/src/lib.rs:1807:13: attempt to add with overflowwithout them, it wraps silently, which is the worse half. The URL ends up with
a serialization of 4294967299 bytes and offsets that no longer point where they
claim to:
It stays memory-safe, but
query()andfragment()return slices of whateverhappens to sit at the wrapped offsets.
The fix
@Manishearth said on the issue "Yeah, we should return an error here".
set_pathreturns
(), and changing that is a breaking change for 2.x, so what I've doneinstead is the closest non-breaking equivalent: notice that the result cannot be
represented and leave the URL exactly as it was, rather than panicking or
corrupting it. That matches how several other setters behave on input they can't
apply, and it's documented on the method now. Happy to rework this as a fallible
API (or a new
try_set_path) if you'd rather have a real error channel — justsay the word.
Percent-encoding grows the input by at most a factor of three, so for any
ordinary path an upper bound on the result is known before the reparse and is
trivially under
u32::MAX. In that case nothing changes and nothing extra isallocated. Only when that upper bound doesn't fit does
set_pathkeep a copy ofthe old path so it can put it back if the reparse really did produce something
too large. So there's no cost on the normal path, and no rejection of large-but-
representable paths either.
Testing
test_set_path_too_longinurl/tests/unit.rsis the repro from the issue. Itfails on
main(attempt to add with overflowatlib.rs:1807) and passeswith this change. It needs ~6 GB of RAM to allocate the input and the oversized
serialization, so it's
#[ignore]d and gated on 64-bit, same aspunycode::huge_encode. To run it:The rollback path is hard to reach from a cheap test, so I checked it separately
by forcing both new conditions to always fire and confirming
set_pathbecomesan exact no-op for special, non-special and cannot-be-a-base URLs with and
without a query and fragment, with the reparsed URL still comparing equal (i.e.
the offsets stay consistent). I also ran the whole suite with the snapshot
branch forced on, to confirm it doesn't disturb the normal path.
cargo test,cargo test --no-default-features --features=alloc,cargo fmt --all --checkandcargo clippy --workspace --all-targets -- -D warningsare all clean.
Not covered
restore_after_pathis also called fromPathSegmentsMut'sDrop, which canreach the same overflow via a huge
push(). Rolling back there is a differentshape of change (
Dropcan't report anything and the type mutates incrementally),so I left it out to keep this focused on the reported bug. Let me know if you'd
like it handled in the same PR.