Properly track (and mark) truncated store entries #909
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Squid used an error-prone approach to identifying truncated responses:
The response is treated as whole[1] unless somebody remembers to mark it
as truncated. This dangerous default naturally resulted in bugs where
truncated responses are treated as complete under various conditions,
This change reverses that approach: Responses not explicitly marked as
whole are treated as truncated. This change affects all Squid-server
FwsState-dispatched communications: HTTP, FTP, Gopher, and WHOIS. It
also affects responses received from the adaptation services.
Squid still tries to deliver responses with truncated bodies to clients
in most cases (no changes are expected/intended in that area).
[1]: A better word to describe a "whole" response would be "complete",
but several key Squid APIs use "complete" to mean "no more content
is coming", misleading developers into thinking that a "completed"
response has all the expected bytes and may be cached/shared/etc.
Responses with truncated HTTP headers
This change also starts treating responses with truncated HTTP headers
(i.e. no CRLF after all the field-lines) as malformed, replacing them
with an HTTP 502 ERR_INVALID_RESP error (same as any other HTTP response
with malformed headers would get).
Since Bug 2879 (commit da6c841 and earlier v2-only changes), Squid was
"fixing" such truncated headers by adding an extra CRLF sequence and
re-parsing them. Depending on the truncation circumstances, that old
workaround could result in rather bad outcomes for Squid and its
clients. Hopefully, we no longer need to work around such bugs. If we
do, we should do it in a safer manner and with admin permission.
Related access-logging improvements
Transactions that failed due to origin server or peer timeout (a common
source of truncation) are now logged with a _TIMEOUT %Ss suffix and
ERR_READ_TIMEOUT/WITH_SRV %err_code/%err_detail.
Transactions prematurely canceled by Squid during client-Squid
communication (usually due to various timeouts) now have WITH_CLT
default %err_detail. This detail helps distinguish otherwise
similarly-logged problems that may happen when talking to the client or
to the origin server/peer.
Other
FwdState now (indirectly) complete()s truncated entries _after_
releasing/adjusting them so that invokeHandlers() and others do not get
a dangerous glimpse at a seemingly OK entry before its release().
yadij
requested changes
Oct 6, 2021
so that it could be applied as an independent change.
* removed a default member initializer (avoid mixing member initialization styles for FwdState) * renamed WITH_SRV/WITH_CLT to their non-abbreviated analogs
rousskov
added
the
S-waiting-for-reviewer
yadij
added
S-waiting-for-author
thus addressing a review request.
rousskov
added
S-waiting-for-reviewer
yadij
approved these changes
Oct 9, 2021
yadij
added
M-cleared-for-merge
rousskov
approved these changes
Oct 10, 2021
squid-anubis
pushed a commit
that referenced
this pull request
Oct 10, 2021
## Responses with truncated bodies
Squid used an error-prone approach to identifying truncated responses:
The response is treated as whole[^1] unless somebody remembers to mark
it as truncated. This dangerous default naturally resulted in bugs where
truncated responses are treated as complete under various conditions.
This change reverses that approach: Responses not explicitly marked as
whole are treated as truncated. This change affects all Squid-server
FwsState-dispatched communications: HTTP, FTP, Gopher, and WHOIS. It
also affects responses received from the adaptation services.
Squid still tries to deliver responses with truncated bodies to clients
in most cases (no changes are expected/intended in that area).
[^1]: A better word to describe a "whole" response would be "complete",
but several key Squid APIs use "complete" to mean "no more content
is coming", misleading developers into thinking that a "completed"
response has all the expected bytes and may be cached/shared/etc.
## Related access-logging improvements
Transactions that failed due to origin server or peer timeout (a common
source of truncation) are now logged with a _TIMEOUT %Ss suffix and
ERR_READ_TIMEOUT/WITH_SRV %err_code/%err_detail.
Transactions prematurely canceled by Squid during client-Squid
communication (usually due to various timeouts) now have WITH_CLT
default %err_detail. This detail helps distinguish otherwise
similarly-logged problems that may happen when talking to the client or
to the origin server/peer.
## Other
FwdState now (indirectly) complete()s truncated entries _after_
releasing/adjusting them so that invokeHandlers() and others do not get
a dangerous glimpse at a seemingly OK entry before its release().
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
squidadm
pushed a commit
to squidadm/squid
that referenced
this pull request
Jan 23, 2022
…che#909) Squid used an error-prone approach to identifying truncated responses: The response is treated as whole[^1] unless somebody remembers to mark it as truncated. This dangerous default naturally resulted in bugs where truncated responses are treated as complete under various conditions. This change reverses that approach: Responses not explicitly marked as whole are treated as truncated. This change affects all Squid-server FwsState-dispatched communications: HTTP, FTP, Gopher, and WHOIS. It also affects responses received from the adaptation services. Squid still tries to deliver responses with truncated bodies to clients in most cases (no changes are expected/intended in that area). [^1]: A better word to describe a "whole" response would be "complete", but several key Squid APIs use "complete" to mean "no more content is coming", misleading developers into thinking that a "completed" response has all the expected bytes and may be cached/shared/etc. Transactions that failed due to origin server or peer timeout (a common source of truncation) are now logged with a _TIMEOUT %Ss suffix and ERR_READ_TIMEOUT/WITH_SRV %err_code/%err_detail. Transactions prematurely canceled by Squid during client-Squid communication (usually due to various timeouts) now have WITH_CLT default %err_detail. This detail helps distinguish otherwise similarly-logged problems that may happen when talking to the client or to the origin server/peer. FwdState now (indirectly) complete()s truncated entries _after_ releasing/adjusting them so that invokeHandlers() and others do not get a dangerous glimpse at a seemingly OK entry before its release().
yadij
pushed a commit
that referenced
this pull request
Jan 24, 2022
Squid used an error-prone approach to identifying truncated responses:
The response is treated as whole[^1] unless somebody remembers to mark
it as truncated. This dangerous default naturally resulted in bugs where
truncated responses are treated as complete under various conditions.
This change reverses that approach: Responses not explicitly marked as
whole are treated as truncated. This change affects all Squid-server
FwsState-dispatched communications: HTTP, FTP, Gopher, and WHOIS. It
also affects responses received from the adaptation services.
Squid still tries to deliver responses with truncated bodies to clients
in most cases (no changes are expected/intended in that area).
[^1]: A better word to describe a "whole" response would be "complete",
but several key Squid APIs use "complete" to mean "no more content
is coming", misleading developers into thinking that a "completed"
response has all the expected bytes and may be cached/shared/etc.
Transactions that failed due to origin server or peer timeout (a common
source of truncation) are now logged with a _TIMEOUT %Ss suffix and
ERR_READ_TIMEOUT/WITH_SRV %err_code/%err_detail.
Transactions prematurely canceled by Squid during client-Squid
communication (usually due to various timeouts) now have WITH_CLT
default %err_detail. This detail helps distinguish otherwise
similarly-logged problems that may happen when talking to the client or
to the origin server/peer.
FwdState now (indirectly) complete()s truncated entries _after_
releasing/adjusting them so that invokeHandlers() and others do not get
a dangerous glimpse at a seemingly OK entry before its release().
ShmayaFrankel
added a commit
to EricomSoftwareLtd/squid
that referenced
this pull request
Feb 7, 2022
…quid-cache#909)" This reverts commit ed99667.
ShmayaFrankel
added a commit
to EricomSoftwareLtd/squid
that referenced
this pull request
Feb 15, 2022
…quid-cache#909)" This reverts commit ed99667.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Responses with truncated bodies
Squid used an error-prone approach to identifying truncated responses:
The response is treated as whole1 unless somebody remembers to mark
it as truncated. This dangerous default naturally resulted in bugs where
truncated responses are treated as complete under various conditions.
This change reverses that approach: Responses not explicitly marked as
whole are treated as truncated. This change affects all Squid-server
FwsState-dispatched communications: HTTP, FTP, Gopher, and WHOIS. It
also affects responses received from the adaptation services.
Squid still tries to deliver responses with truncated bodies to clients
in most cases (no changes are expected/intended in that area).
Related access-logging improvements
Transactions that failed due to origin server or peer timeout (a common
source of truncation) are now logged with a _TIMEOUT %Ss suffix and
ERR_READ_TIMEOUT/WITH_SRV %err_code/%err_detail.
Transactions prematurely canceled by Squid during client-Squid
communication (usually due to various timeouts) now have WITH_CLT
default %err_detail. This detail helps distinguish otherwise
similarly-logged problems that may happen when talking to the client or
to the origin server/peer.
Other
FwdState now (indirectly) complete()s truncated entries after
releasing/adjusting them so that invokeHandlers() and others do not get
a dangerous glimpse at a seemingly OK entry before its release().
Footnotes
A better word to describe a "whole" response would be "complete",
but several key Squid APIs use "complete" to mean "no more content
is coming", misleading developers into thinking that a "completed"
response has all the expected bytes and may be cached/shared/etc. ↩