3 ms·

Any idea why this code is using two measures of the body at all? Per the RFC, in HTTP/1.1, the Content-Length is the authoritative information for how long the

Any idea why this code is using two measures of the body at all? Per the RFC, in HTTP/1.1, the Content-Length is the authoritative information for how long the HTTP request/response body is. Any data on the wire past the content length is part of another message, by definition. Conversely, if Content-Length is larger then rx_message.BodyLength, that means that we didn't yet receive the whole message, so we should either wait more, or issue a timeout error. Either way, rx_message.BodyLength is a bogus value if it's not guaranteed to be equal to Content-Length. On the other hand, if you want to be more permissive, then why look at the Content-Length header at all? Just use rx_message.BodyLength as the buffer size and try to interpret all the data on the wire as the received message. The current version of the code is needlessly complex, and that is how bugs like this make it in.

oefrha 3y ago
Looking at the commit in isolation can be somewhat misleading. If you look at the surrounding code https://github.com/rhboot/shim/blob/0226b56513b2b8bd5fd281bce77c40c9bf07c66d/httpboot.c#L572-L638 https://github.com/rhboot/shim/blob/0226b56513b2b8bd5fd281bc..., it's receiving chunks of data in a loop, checking to make sure new data does not exceed the buffer capacity (determined by Content-Length) each time. However, previously it wasn't checking that on the very first read outside the loop, hence the bug. I don't see any final check on downloaded == *buf_size (Content-Length) though. Violating that could indicate premature connection close.
- tsimionescu 3y ago
  Oh, thanks for that extra context. I did look a bit further down, but didn't see the loop... This makes much more sense. Looking at the loop though, I think it does ensure that downloaded == *buf_size: the loop continues while downloaded < *buf_size, and there is a different check for not overflowing. So, overall, downloaded must be exactly equal to *buf_size to exit the loop without an error. So, this seems OK overall.
  - oefrha 3y ago
    Oh yeah, thought I saw a break, apparently not.

oefrha 3y ago

Looking at the commit in isolation can be somewhat misleading. If you look at the surrounding code https://github.com/rhboot/shim/blob/0226b56513b2b8bd5fd281bce77c40c9bf07c66d/httpboot.c#L572-L638 https://github.com/rhboot/shim/blob/0226b56513b2b8bd5fd281bc..., it's receiving chunks of data in a loop, checking to make sure new data does not exceed the buffer capacity (determined by Content-Length) each time. However, previously it wasn't checking that on the very first read outside the loop, hence the bug. I don't see any final check on downloaded == *buf_size (Content-Length) though. Violating that could indicate premature connection close.

tsimionescu 3y ago
Oh, thanks for that extra context. I did look a bit further down, but didn't see the loop... This makes much more sense. Looking at the loop though, I think it does ensure that downloaded == *buf_size: the loop continues while downloaded < *buf_size, and there is a different check for not overflowing. So, overall, downloaded must be exactly equal to *buf_size to exit the loop without an error. So, this seems OK overall.
- oefrha 3y ago
  Oh yeah, thought I saw a break, apparently not.