fix: Avoid panics when content is longer than content length header#8
fix: Avoid panics when content is longer than content length header#8
Conversation
konstin
changed the title
konstin
force-pushed
the
konsti/bounds-checks
branch
from
d55f528 to
461909f
Compare
Fixes astral-sh/uv#18316 This fix handles two cases: * The server sends more bytes than initially promised in the `Content-Length` header * The server sends more or less bytes than the `content-range` indicated As a side effect, we remove the requirement that the initial response must be towards the end (`-len:`).
konstin
force-pushed
the
konsti/bounds-checks
branch
from
461909f to
bba9d11
Compare
EliteTK
left a comment
EliteTK
left a comment
There was a problem hiding this comment.
I believe this would fix it. Can't see any fundamental problems with this fix aside from https://github.com/astral-sh/async_http_range_reader/pull/8/changes#r2902785393.
But I feel there are a few improvements which could be made to the error handling.
When we encounter this error in uv, do we switch to just normal non-range requests? If not, it might need a follow up PR for that since it wouldn't be a good fix if instead of panicking we just error in this case without trying the slow path.
There is an argument to be made that for the case in astral-sh/uv#18316, since the range we get back covers the entire memory map, we could just accept it. It would fill the memory map and our reads can just return from this memory map with no further requests.
I guess then we can't warn users about this, although I am not sure if it's useful to be noisy about warning about this anyway...
| offset = byte_range.end; | ||
| offset += bytes.len() as u64; |
There was a problem hiding this comment.
nit: I liked the previous version of this line better :)
src/lib.rs
Outdated
| if offset > end_exclusive { | ||
| state.error = Some(AsyncHttpRangeReaderError::ContentLengthMismatch { | ||
| expected: end_exclusive - start, | ||
| actual: offset - start, |
There was a problem hiding this comment.
This isn't quite true. The actual content length if the response is chunked and there's no content length header is unknowable. And regardless, if there is a content length, that's what the actual content length would be.
| let _ = state_tx.send(state); | ||
| break 'outer; | ||
| } | ||
|
|
There was a problem hiding this comment.
About here is where we should check that the response's range header matches what we requested.
This would catch the case where a server replies with a range of the right size, but at the wrong offset, which would be a mangled reply, but still entirely possible.
Co-authored-by: Tomasz Kramkowski <tomasz@kramkow.ski>
We match on the upstream error type: https://github.com/astral-sh/uv/blob/401661ee22048d98a0e58c3be9bf3fd2d162e062/crates/uv-client/src/error.rs#L245. We have to extend this, I would at least make the error loud, there's something really bad going on with the user's server and they get a severe performance degradation due to that, that they'd otherwise attribute to uv and never get a chance of fixing internally.
Our first request is from the end of the file, so if we'd get the whole file, we'd still overflow, we'd need a different path for that. (Treating this response as a regular streaming response?) |
I get that, but also some of our users just won't be able to do anything about this, so they'd get warnings they have to ignore. It seems like a difficult balance. |
|
Tbh if you're using an index server that's so badly broken it advertises range requests but isn't capable of returning correct responses (not to mention PEP 658 support), causing a massive degradation of performance in uv, complaining about this seems appropriate; it's not hard to come by a server or reverse proxy that implements this correctly, and I find it almost more reasonable to straight up reject a registry that doesn't even clear this minimal bar - It could just not advertise range request support! |
2 participants
Fixes astral-sh/uv#18316
This fix handles two cases:
Content-Lengthheadercontent-rangeindicatedAs a side effect, we remove the requirement that the initial response must be towards the end (
-len:).