ICP: Fix HttpRequest lifetime for ICP v3 queries#2377
Closed
rousskov wants to merge 2 commits intosquid-cache:masterfrom
Closed
ICP: Fix HttpRequest lifetime for ICP v3 queries#2377rousskov wants to merge 2 commits intosquid-cache:masterfrom
rousskov wants to merge 2 commits intosquid-cache:masterfrom
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 and age 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.
And resolved seemingly simple/safe conflicts.
rousskov
added
the
S-could-use-an-approval
yadij
requested changes
Feb 19, 2026
yadij
added
S-waiting-for-author
Contributor
Author
What makes you think that? AFAICT,
DelayedUdpSend objects do not store HttpRequests. Also, ICPState-derived objects are always allocated on stack and, hence, are never queued for any asynchronous operations. |
rousskov
added
S-waiting-for-reviewer
Contributor
|
You are right, sorry. I only looked at the header and member type as raw-pointer. Approving now. |
yadij
approved these changes
Feb 19, 2026
yadij
added
M-cleared-for-merge
rousskov
added
the
S-could-use-an-approval
squid-anubis
pushed a commit
that referenced
this pull request
Feb 20, 2026
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-waiting-staging-checks
squid-anubis
added
M-merged
rousskov
removed
the
S-could-use-an-approval
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
3 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.