ICP: Fix HttpRequest lifetime for ICP v3 queries#2387
Merged
yadij merged 1 commit intosquid-cache:v7from Mar 12, 2026
Merged
Conversation
ACLFilledChecklist correctly locks and unlocks HttpRequest. Thus, when given an unlocked request object, an on-stack checklist destroys it. Upon icpAccessAllowed() return, Squid uses the destroyed request object. This bug was probably introduced in 2003 commit 8000a96 that started automatically unlocking requests in ACLChecklist destructor. However, the bug did not affect allowed ICP v3 queries until 2007 commit f72fb56 started _using_ the request object for them. 2005 commit 319bf5a fixed an equivalent ICP v2 bug for denied queries but missed the ICP v3 case. The scope, age, and effect of this bug imply that Squid v3+ deployments receive no ICP v3 queries since 2007 (or earlier). Squid itself does not send ICP v3 messages, responding with ICP v2 replies to ICP v3 queries. TODO: Consider dropping ICP v3 support. Also moved icpAccessAllowed() inside icpGetRequest() to deduplicate code and reduce the risk of allowing a request without consulting icp_access.
squid-anubis
added
the
M-failed-description
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
rousskov
changed the title
squid-anubis
removed
the
M-failed-description
Contributor
|
I copied original commit 49f47cc message into PR title/description. |
Contributor
Author
Thanks. For some reason my editor kept messing up formatting |
Contributor
Author
|
waiting for approval, can't merge without |
squid-anubis
added
the
M-failed-other
yadij
approved these changes
Mar 12, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
ACLFilledChecklist correctly locks and unlocks HttpRequest. Thus, when
given an unlocked request object, an on-stack checklist destroys it.
Upon icpAccessAllowed() return, Squid uses the destroyed request object.
This bug was probably introduced in 2003 commit 8000a96 that started
automatically unlocking requests in ACLChecklist destructor. However,
the bug did not affect allowed ICP v3 queries until 2007 commit f72fb56
started using the request object for them. 2005 commit 319bf5a fixed
an equivalent ICP v2 bug for denied queries but missed the ICP v3 case.
The scope, age, and effect of this bug imply that Squid v3+ deployments
receive no ICP v3 queries since 2007 (or earlier). Squid itself does not
send ICP v3 messages, responding with ICP v2 replies to ICP v3 queries.
TODO: Consider dropping ICP v3 support.
Also moved icpAccessAllowed() inside icpGetRequest() to deduplicate code
and reduce the risk of allowing a request without consulting icp_access.