Skip to content

Fix u32 overflow in set_path on oversized paths - #1152

Open
pacocartones wants to merge 1 commit into
servo:mainfrom
pacocartones:fix-1106-set-path-overflow
Open

Fix u32 overflow in set_path on oversized paths#1152
pacocartones wants to merge 1 commit into
servo:mainfrom
pacocartones:fix-1106-set-path-overflow

Conversation

@pacocartones

Copy link
Copy Markdown

Fixes #1106.

The bug

Url keeps query_start and fragment_start as u32 offsets into the
serialization. set_path reparses the path and then calls
restore_after_path, which rebases those offsets onto the new end of the path
with plain u32 arithmetic:

let adjust = |index: &mut u32| {
    *index -= old_after_path_position;
    *index += new_after_path_position;
};

Nothing checks that the result still fits, so a path whose percent-encoded form
pushes the serialization up against u32::MAX overflows that addition. With the
repro from the issue on main (00a6ce5):

  • with overflow checks on: panicked at url/src/lib.rs:1807:13: attempt to add with overflow

  • without 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:

    serialization len = 4294967299 (u32::MAX = 4294967295)
    query()    = Some("h")                     // was "a"
    fragment() = Some("tp://a/%20%20%20%20...") // was "b", now ~4 GB of the path
    

    It stays memory-safe, but query() and fragment() return slices of whatever
    happens to sit at the wrapped offsets.

The fix

@Manishearth said on the issue "Yeah, we should return an error here". set_path
returns (), and changing that is a breaking change for 2.x, so what I've done
instead 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 — just
say 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 is
allocated. Only when that upper bound doesn't fit does set_path keep a copy of
the 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_long in url/tests/unit.rs is the repro from the issue. It
    fails on main (attempt to add with overflow at lib.rs:1807) and passes
    with 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 as
    punycode::huge_encode. To run it:

    RUSTFLAGS="-C overflow-checks=on" cargo test --release -p url --test unit -- --ignored test_set_path_too_long
    
  • 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_path becomes
    an 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 --check and cargo clippy --workspace --all-targets -- -D warnings
    are all clean.

Not covered

restore_after_path is also called from PathSegmentsMut's Drop, which can
reach the same overflow via a huge push(). Rolling back there is a different
shape of change (Drop can'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.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set_path panics on oversized input instead of returning an error

1 participant