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.

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.

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:

Because HttpReply::bodySize() type is int64_t, and HttpBody::contentSize() is size_t.

Notice that with the correction maximumSize and all the conflated buffer confusion disappears.

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() is size_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_t maximum is smaller than int64_t maximum, we could set maximumOffset based on size_t maximum, but that would probably break some existing benign transactions for no good reason -- as far as we know, Squid can handle offsets with size_t maximum values on those platforms today, with or without this PR changes.
• On platforms where size_t maximum is bigger than int64_t maximum (i.e. a common/primary case), it is the latter/smaller maximum that matters. Adding size_t into maximumOffset min() 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 maximumSize and 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.

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.

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 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.

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 maximumSize and 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.

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.

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. 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.

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 maximumSize and 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.

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.

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.

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.

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.

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.

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. ]

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 - maxPossibleLength is 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.offset separately, but doing so is insufficient and misleading.
• We could check spec.length separately, 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.length

So: offset > MAX_INT - maxPossibleLength - actualLength is 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 + length does 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.

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?

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.

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.

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 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.

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.

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.