Handle even more Content-Range responses - #2461
Conversation
* resp-range-spec case was protected by subsequent size_diff()<=0 check. * Request range-spec case resulted in incorrect/negative length despite successful HttpHdrRangeSpec::parseInit() outcome. I do not know what the consequences of that inconsistency are.
The cast is necessary to avoid compiler `-Wsign-compare` warnings because `wrote` is unsigned. TODO: Consider adding and using Increment[able](offset, delta) and/or reusing existing IncreaseSum(offset, delta).
This version is not just more compact: It is checking inputs _before_ `mem_hdr::write()` modifies anything. Its primary problem is that it cannot distinguish overflowing `writeBuffer.offset + writeBuffer.length` sum from negative `writeBuffer.length`. However, it can be argued that all three of those are likely the result of integer overflowing in caller's code. N.B. `unionNotEmpty()` call asserts that offset is not negative, so this change does not really drop that `mem_hdr::write()` parameter check.
| static_assert(std::numeric_limits<decltype(range->spec.offset)>::max() >= std::numeric_limits<int64_t>::max()); | ||
| const auto maximumSize = int64_t(1024)*1024*1024*1024; // no in-memory Squid object/buffer size can exceed 1 TiB | ||
| const auto maximumOffset = std::numeric_limits<int64_t>::max() - maximumSize; | ||
| if (range->spec.length > maximumOffset || range->spec.offset > maximumOffset - range->spec.length) { |
There was a problem hiding this comment.
This condition is difficult for humans to grok quickly. It is essentially a safe version of
if (range->spec.offset + range->spec.length > maximumOffset)There are probably other places in code where we do this, although most are going to compare with std::numerical_limits::max() rather than some custom maximumOffset. Please let me know if I should wrap this logic in a reusable function.
There was a problem hiding this comment.
FYI the only confusing thing here is the calculation used to generate value for maximumOffset. Subtracting max(length) from it before comparing length to the remainder. i.e. length > N-max(length). Once length has been accounted for the remainder value should have no relevance to length.
There was a problem hiding this comment.
FYI the only confusing thing here is the calculation used to generate value for maximumOffset. Subtracting max(length) from it before comparing length to the remainder. i.e. length > N-max(length). Once length has been accounted for the remainder value should have no relevance to length.
Sorry, I do not understand what the last two sentences in the above comment are saying or what changes this change request is requesting (if any). Please detail/rephrase if this is still relevant after the clarifications in the other/primary change request thread.
There was a problem hiding this comment.
Take the if-statment conditions:
range->spec.length > maximumOffset
So length > MAX_INT - maxPossibleLength is testing that length is smaller than memory it wont be put into.
When what is needed is a check that length is small enough for Squid to process.
That would be range->spec.length > maximumSize
range->spec.offset > maximumOffset - range->spec.length
So: offset > MAX_INT - maxPossibleLength - actualLength is accounting for length at least twice.
When what is needed is to ensure that offset + length does not overflow during processing.
That would be range->spec.offset > (MAX_INT-1) - range->spec.length.
[UPDATE: these checks should really be split into two if-statements with unique error messages relating to the length vs offset which is found to be too big. ]
There was a problem hiding this comment.
Take the if-statment conditions:
range->spec.length > maximumOffset
We should not interpret this part of the actual condition in isolation. As I said when posting this PR, this part of the condition exists simply because C++ cannot express the actual condition we want to test without overflowing (or underflowing):
- We want to say:
spec.offset + spec.length > maximumOffset, - but to prevent C++ overflows, we use its mathematically equivalent variant:
spec.offset > maximumOffset - spec.length, - and we check subtraction on the right from
>for C++ underflows first:spec.length > maximumOffset || spec.offset > maximumOffset - spec.length
The three conditions in the above three bullets are mathematically equivalent, but the first one may overflow in C++ code, and the second one might underflow. Separating the two ORed expressions in the third/proposed condition and treating each as a stand-alone check creates more problems than it solves.
Again, if this math is considered difficult to grok, we can add a wrapper function, so that the high-level test becomes something like this:
if (SumExceeds(spec.offset, spec.length, maximumOffset))
debugs(68, 2, "huge content-range-spec near: '" << str << "'");or, with even more out-of-scope effort, we can achieve more readable code similar to this:
if (BigSum(spec.offset, spec.length) > maximumOffset)
debugs(68, 2, "huge content-range-spec near: '" << str << "'");I will implement any of the two changes sketched above if you request them. Should I?
So
length > MAX_INT - maxPossibleLengthis testing that length is smaller than memory it wont be put into.
The proposed range->spec.length > maximumOffset part of the condition is testing that the following subtraction in the second part of the condition will not underflow (especially after we improve this code to use unsigned offsets): maximumOffset - range->spec.length. This part is just basic integer safety precaution/math, not some deep HTTP or Squid code semantics check.
When what is needed is a check that length is small enough for Squid to process. That would be
range->spec.length > maximumSize
- We could check
spec.offsetseparately, but doing so is insufficient and misleading. - We could check
spec.lengthseparately, but doing so is insufficient and misleading. - We do check the end offset (i.e.
spec.offset + spec.length), which is necessary and sufficient (but is not trivial due to C++ integer math limitations).
A range->spec.length > maximumSize condition (mentioned in the change request part quoted above) would not be enough to cover all cases. The content range length can be small (e.g., 7 bytes), but the corresponding huge offsets can still overwhelm Squid code. It is the offset absolute values we care about here, not the number of bytes in the received content range; spec.length is the latter.
range->spec.offset > maximumOffset - range->spec.lengthSo:
offset > MAX_INT - maxPossibleLength - actualLengthis accounting for length at least twice.
No, it does not: In the hypothetical code (not proposed in this PR) quoted above, "length" in maxPossibleLength and "length" in actualLength are actually different lengths.
When what is needed is to ensure that
offset + lengthdoes not overflow during processing.
No, that is not what is needed to ensure. The problem this PR is solving may happen even if spec.offset + spec.length does not overflow. Again, please see the corresponding email for details.
That would be
range->spec.offset > (MAX_INT-1) - range->spec.length.
True but pretty much irrelevant because we need to ban more overflows than just overflows in the spec.offset + spec.length expressions.
[UPDATE: these checks should really be split into two if-statements with unique error messages relating to the length vs offset which is found to be too big. ]
The above request is based on a false assumption that the proposed check is meant for testing offset and length individually or separately. In reality, the proposed check tests the end offset (a single entity). It is a single check for all possible byte offsets (split into two conditions to prevent C++ integer overflows and underflows). Splitting the proposed single check into two checks will create more problems. If you propose a specific split, I should be able to identify and detail those problems, but all that will take time and is very unlikely to improve Squid. I recommend approving this PR instead.
| return false; | ||
| } | ||
|
|
||
| assert (writeBuffer.offset >= 0); |
There was a problem hiding this comment.
This assertion is repeated in the above unionNotEmpty() call, so we are not really removing it here. The new Assure() call below still covers negative offsets, among other things.
| // Store I/O adds partial content offsets to the size of various objects and | ||
| // buffers (e.g., Store metadata, serialized HTTP headers, mem_node::data, | ||
| // and Store I/O buffer). Most such sums do not check for overflows, so we |
There was a problem hiding this comment.
What does HTTP Range header syntax validity have to do with Squid internal Store I/O buffer management?
AFAIK the only overlap is concept and terminology.
There was a problem hiding this comment.
What does HTTP Range header syntax validity have to do with Squid internal Store I/O buffer management?
The proposed additional checks do not check HTTP Range header syntax validity. The proposed code checks whether the (successfully parsed) value is safe to use in the rest of Squid code. httpHdrContRangeParseInit() scope is not limited to syntax validation.
| // further assume that most offsets use int64_t or a larger integer type. | ||
| static_assert(std::numeric_limits<decltype(range->spec.offset)>::max() >= std::numeric_limits<int64_t>::max()); | ||
| const auto maximumSize = int64_t(1024)*1024*1024*1024; // no in-memory Squid object/buffer size can exceed 1 TiB | ||
| const auto maximumOffset = std::numeric_limits<int64_t>::max() - maximumSize; |
There was a problem hiding this comment.
The spec.offset limit is exactly one less than maximum HTTP resource size (elength limit), not the message content size (spec.length limit).
Comparison against elength is done later without reference to this constant. Which means the value here should actually be:
| const auto maximumOffset = std::numeric_limits<int64_t>::max() - maximumSize; | |
| const auto maximumOffset = min(std::numeric_limits<int64_t>::max(), std::numeric_limits<size_t>::max()) - 1; |
Because HttpReply::bodySize() type is int64_t, and HttpBody::contentSize() is size_t.
There was a problem hiding this comment.
Notice that with the correction maximumSize and all the conflated buffer confusion disappears.
There was a problem hiding this comment.
The spec.offset limit is exactly one less than maximum HTTP resource size (elength limit), not the message content size (spec.length limit).
elength-related spec.offset checks are unrelated to the problem this PR is solving. They already exist lower in this code. This PR is about checking the maximum offset values. elength may be bigger than the maximum offset values Squid actually supports today. To fix the problem this PR is solving, we do not need to reject Content-Range headers with huge elength values and small content offsets, for example.
Because ...
HttpBody::contentSize()issize_t.
Squid does not use HttpBody for the cases relevant to this PR. HttpBody is a special class used for "internal" cases like Squid-generated error responses. Those do not have Content-Range headers. Furthermore:
- On platforms where
size_tmaximum is smaller thanint64_tmaximum, we could set maximumOffset based onsize_tmaximum, but that would probably break some existing benign transactions for no good reason -- as far as we know, Squid can handle offsets withsize_tmaximum values on those platforms today, with or without this PR changes. - On platforms where
size_tmaximum is bigger thanint64_tmaximum (i.e. a common/primary case), it is the latter/smaller maximum that matters. Addingsize_tintomaximumOffsetmin()calculation would not change anything on those platforms.
In summary, HttpBody and size_t are not really relevant to the problem this PR is solving and cannot improve the solution.
Suggested code:
const auto maximumOffset = min(...max(), ...max()) - 1;The above suggestion does not address the problem this PR is addressing. Subtracting 1 is not enough because, as PR-added C++ comment explicitly says, we need to be ready for adding various buffer sizes and offsets that naturally exceed 1 in most cases.
Notice that with the correction
maximumSizeand all the conflated buffer confusion disappears.
... along with the fix for the problem this PR is solving. maximumSize is the key here. maximumSize of 1 would not work because most buffer sizes/offsets being added to spec-derived values exceed 1.
There was a problem hiding this comment.
The spec.offset limit is exactly one less than maximum HTTP resource size (elength limit), not the message content size (spec.length limit).
elength-relatedspec.offsetchecks are unrelated to the problem this PR is solving. They already exist lower in this code. This PR is about checking the maximum offset values.elengthmay be bigger than the maximum offset values Squid actually supports today. To fix the problem this PR is solving, we do not need to rejectContent-Rangeheaders with hugeelengthvalues and small content offsets, for example.
That was my point. This PR code as written rejects cases where the object is larger than Squid can transfer in its entirety, but in chunks small enough that Squid does already handle fine. For example; science and medical datasets have Petta-byte large objects going through in GiB or TiB sized blocks.
Suggested code:
const auto maximumOffset = min(...max(), ...max()) - 1;
The above suggestion does not address the problem this PR is addressing. Subtracting
1is not enough because, as PR-added C++ comment explicitly says, we need to be ready for adding various buffer sizes and offsets that naturally exceed 1 in most cases.
It is not appropriate for this constant to secretly try to account for a run-time length value. That is done as part of the if-statement condition, where it should be.
Notice that with the correction
maximumSizeand all the conflated buffer confusion disappears.... along with the fix for the problem this PR is solving.
maximumSizeis the key here.maximumSizeof1would not work because most buffer sizes/offsets being added tospec-derived values exceed1.
I am not at any point suggesting that maximumSize should be 1. I am requesting that these maximumFoo constants actually contain the limit value for their matching Foo parameter.
See https://github.com/squid-cache/squid/pull/2461/changes#r3649826148.
There was a problem hiding this comment.
This PR code as written rejects cases where the object is larger than Squid can transfer in its entirety, but in chunks small enough that Squid does already handle fine.
What makes you think that?
For example; science and medical datasets have Petta-byte large objects going through in GiB or TiB sized blocks.
AFAICT, PR code does not ban Petta-byte large objects going through in GiB or TiB sized blocks.
Can you give a specific example of a Content-Range value that this PR rejects but that unpatched Squid (i.e. official code) handles correctly?
Suggested code:
const auto maximumOffset = min(...max(), ...max()) - 1;
The above suggestion does not address the problem this PR is addressing. Subtracting1is not enough because, as PR-added C++ comment explicitly says, we need to be ready for adding various buffer sizes and offsets that naturally exceed 1 in most cases.
It is not appropriate for this constant to secretly try to account for a run-time length value. That is done as part of the if-statement condition, where it should be.
This is not about "length" it is about "offset", and I see no secrets hidden in PR code. I do not know how you want the "if-statement condition" look, so I cannot commit or reject the corresponding change.
Notice that with the correction
maximumSizeand all the conflated buffer confusion disappears.
... along with the fix for the problem this PR is solving.
maximumSizeis the key here.maximumSizeof1would not work because most buffer sizes/offsets being added tospec-derived values exceed1.
I am not at any point suggesting that
maximumSizeshould be1. I am requesting that thesemaximumFooconstants actually contain the limit value for their matchingFooparameter.
AFAICT, proposed constants already contain appropriate or "matching" values, so I cannot tell what changes you are requesting. Please be more specific. AFAICT, your definition of "maximum offset" in maximumOffset differs from PR's definition, but I cannot tell what corresponding code changes you want me to implement, so I cannot commit or reject them. The suggestion that started this change request is wrong or incomplete (as detailed earlier).
See https://github.com/squid-cache/squid/pull/2461/changes#r3649826148.
The suggestions in that change request have their own problems, but if you think that the two change requests threads are about the same PR problem, then let's resolve at least one of them to save time.
| static_assert(std::numeric_limits<decltype(range->spec.offset)>::max() >= std::numeric_limits<int64_t>::max()); | ||
| const auto maximumSize = int64_t(1024)*1024*1024*1024; // no in-memory Squid object/buffer size can exceed 1 TiB | ||
| const auto maximumOffset = std::numeric_limits<int64_t>::max() - maximumSize; | ||
| if (range->spec.length > maximumOffset || range->spec.offset > maximumOffset - range->spec.length) { |
There was a problem hiding this comment.
FYI the only confusing thing here is the calculation used to generate value for maximumOffset. Subtracting max(length) from it before comparing length to the remainder. i.e. length > N-max(length). Once length has been accounted for the remainder value should have no relevance to length.
| const auto maximumSize = int64_t(1024)*1024*1024*1024; // no in-memory Squid object/buffer size can exceed 1 TiB | ||
| const auto maximumOffset = std::numeric_limits<int64_t>::max() - maximumSize; | ||
| if (range->spec.length > maximumOffset || range->spec.offset > maximumOffset - range->spec.length) { | ||
| debugs(68, 2, "huge content-range-spec near: '" << str << "'"); |
There was a problem hiding this comment.
The purpose of this function is to check validity of HTTP syntax.
I question why simply having "huge" values for range offset are rejected as invalid syntax?
There was a problem hiding this comment.
The purpose of this function is to check validity of HTTP syntax.
The purpose of httpHdrContRangeParseInit() function is to convert the given Content-Range header string value into Squid's internal representation (i.e. an HttpHdrContRange object). That conversion includes parsing (with the corresponding syntax checks) as well as other validation concerns such as syntactically valid but semantically contradicting values and values that Squid cannot support today. The focus of this PR is the latter.
I question why simply having "huge" values for range offset are rejected as invalid syntax?
This question is based on a false premise: Huge values are indeed rejected here, but not because of their syntax (which is actually fine).
The error message text follows the pattern already used in this function. If you would like to see different wording, please suggest a specific replacement.
There was a problem hiding this comment.
As you say the functions purpose is to parse. It is already conflated with HTTP specification validation checks of that parsed input.
My question is about why a function is now being given side effects unrelated to the parse result.
For example; an alternative change would be to add maxLength and maxOffset constants to class HttpHdrRangeSpec where those variables live and check them in the code where overflow may occur. With the view that we can at least try to serve as much of the range as we can before the limit halts transfer.
Or, check the Squid limitations in HttpHeader::getContRange() after the HTTP protocol validations complete.
There was a problem hiding this comment.
As you say the functions purpose is to parse. It is already conflated with HTTP specification validation checks of that parsed input.
I did not say that this function purpose is [just] to parse. I said that this function purpose is to convert, which includes several (related) sub-tasks or sub-purposes, including parsing. I enumerated some of those sub-tasks. This PR does not change this function purpose(s). This PR updates this function in according to its current purpose(s).
It is already conflated with HTTP specification validation checks of that parsed input.
I am not sure I agree that such conflation exists, but even if it does exist, it is outside this PR scope.
My question is about why a function is now being given side effects unrelated to the parse result.
The assertion that this PR "now gives" this function something that official function code does not contain is false. httpHdrContRangeParseInit() function already has code that performs similar checks. Those existing checks are not sufficient. This PR adds some of the missing checks.
For example; an alternative change would be to add
maxLengthandmaxOffsetconstants toclass HttpHdrRangeSpecwhere those variables live and check them in the code where overflow may occur. With the view that we can at least try to serve as much of the range as we can before the limit halts transfer. Or, check the Squid limitations inHttpHeader::getContRange()after the HTTP protocol validations complete.
That alternative is worse than the proposed solution on several levels. For example, it relies on folks remembering to check the limits every time they needed to be checked. As this PR development itself has proven multiple times (e.g., commit 73506d0 and commit 619829e), those cases are very easy to miss even when one is specifically looking for them.
While a long-term solution would be different than the proposed one, the proposed one works reliably in all known cases and is easy to backport. AFAIK, no better small-footprint solution is known at this time.
I was tricked by the complex structure of the chained `if` statements from where the problematic test had to be moved. The `*` case does set `range->elength` to `range_spec_unknown` while other cases reject negative `elength` values.
rousskov
left a comment
There was a problem hiding this comment.
@yadij, if you have not already, please see this email thread for the context of this PR before re-reviewing the proposed changes.
| // Store I/O adds partial content offsets to the size of various objects and | ||
| // buffers (e.g., Store metadata, serialized HTTP headers, mem_node::data, | ||
| // and Store I/O buffer). Most such sums do not check for overflows, so we |
There was a problem hiding this comment.
What does HTTP Range header syntax validity have to do with Squid internal Store I/O buffer management?
The proposed additional checks do not check HTTP Range header syntax validity. The proposed code checks whether the (successfully parsed) value is safe to use in the rest of Squid code. httpHdrContRangeParseInit() scope is not limited to syntax validation.
| static_assert(std::numeric_limits<decltype(range->spec.offset)>::max() >= std::numeric_limits<int64_t>::max()); | ||
| const auto maximumSize = int64_t(1024)*1024*1024*1024; // no in-memory Squid object/buffer size can exceed 1 TiB | ||
| const auto maximumOffset = std::numeric_limits<int64_t>::max() - maximumSize; | ||
| if (range->spec.length > maximumOffset || range->spec.offset > maximumOffset - range->spec.length) { |
There was a problem hiding this comment.
FYI the only confusing thing here is the calculation used to generate value for maximumOffset. Subtracting max(length) from it before comparing length to the remainder. i.e. length > N-max(length). Once length has been accounted for the remainder value should have no relevance to length.
Sorry, I do not understand what the last two sentences in the above comment are saying or what changes this change request is requesting (if any). Please detail/rephrase if this is still relevant after the clarifications in the other/primary change request thread.
| const auto maximumSize = int64_t(1024)*1024*1024*1024; // no in-memory Squid object/buffer size can exceed 1 TiB | ||
| const auto maximumOffset = std::numeric_limits<int64_t>::max() - maximumSize; | ||
| if (range->spec.length > maximumOffset || range->spec.offset > maximumOffset - range->spec.length) { | ||
| debugs(68, 2, "huge content-range-spec near: '" << str << "'"); |
There was a problem hiding this comment.
The purpose of this function is to check validity of HTTP syntax.
The purpose of httpHdrContRangeParseInit() function is to convert the given Content-Range header string value into Squid's internal representation (i.e. an HttpHdrContRange object). That conversion includes parsing (with the corresponding syntax checks) as well as other validation concerns such as syntactically valid but semantically contradicting values and values that Squid cannot support today. The focus of this PR is the latter.
I question why simply having "huge" values for range offset are rejected as invalid syntax?
This question is based on a false premise: Huge values are indeed rejected here, but not because of their syntax (which is actually fine).
The error message text follows the pattern already used in this function. If you would like to see different wording, please suggest a specific replacement.
| // further assume that most offsets use int64_t or a larger integer type. | ||
| static_assert(std::numeric_limits<decltype(range->spec.offset)>::max() >= std::numeric_limits<int64_t>::max()); | ||
| const auto maximumSize = int64_t(1024)*1024*1024*1024; // no in-memory Squid object/buffer size can exceed 1 TiB | ||
| const auto maximumOffset = std::numeric_limits<int64_t>::max() - maximumSize; |
There was a problem hiding this comment.
The spec.offset limit is exactly one less than maximum HTTP resource size (elength limit), not the message content size (spec.length limit).
elength-related spec.offset checks are unrelated to the problem this PR is solving. They already exist lower in this code. This PR is about checking the maximum offset values. elength may be bigger than the maximum offset values Squid actually supports today. To fix the problem this PR is solving, we do not need to reject Content-Range headers with huge elength values and small content offsets, for example.
Because ...
HttpBody::contentSize()issize_t.
Squid does not use HttpBody for the cases relevant to this PR. HttpBody is a special class used for "internal" cases like Squid-generated error responses. Those do not have Content-Range headers. Furthermore:
- On platforms where
size_tmaximum is smaller thanint64_tmaximum, we could set maximumOffset based onsize_tmaximum, but that would probably break some existing benign transactions for no good reason -- as far as we know, Squid can handle offsets withsize_tmaximum values on those platforms today, with or without this PR changes. - On platforms where
size_tmaximum is bigger thanint64_tmaximum (i.e. a common/primary case), it is the latter/smaller maximum that matters. Addingsize_tintomaximumOffsetmin()calculation would not change anything on those platforms.
In summary, HttpBody and size_t are not really relevant to the problem this PR is solving and cannot improve the solution.
Suggested code:
const auto maximumOffset = min(...max(), ...max()) - 1;The above suggestion does not address the problem this PR is addressing. Subtracting 1 is not enough because, as PR-added C++ comment explicitly says, we need to be ready for adding various buffer sizes and offsets that naturally exceed 1 in most cases.
Notice that with the correction
maximumSizeand all the conflated buffer confusion disappears.
... along with the fix for the problem this PR is solving. maximumSize is the key here. maximumSize of 1 would not work because most buffer sizes/offsets being added to spec-derived values exceed 1.
This workaround continues the work started in 2021 commits 8af775e and
7024fb7. A comprehensive long-term fix requires backlogged Client
Streams removal followed by noisy changes replacing raw integers with a
smart Offset class in StoreIOBuffer, HttpHdrRangeSpec, HttpHdrContRange,
ClientHttpRequest::Out, and similar offset-tracking APIs.
Also improved handling of Range and Request-Range request headers.