Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 4842: Memory leak when http_reply_access uses external_acl #424

Conversation

chtsanti
Copy link
Contributor

@chtsanti chtsanti commented Jun 25, 2019

Http::One::Server::handleReply() sets AccessLogEntry::reply which may
already be set. It is already set, for example, when the ACL code
has already called syncAle() because external ACLs require an ALE.

This bug was introduced by commit fbbea66.

This is a Measurement Factory project.

The ACL code updates the AccessLogEntry::reply member when an acl
which is requires the ALE, like the external acls, is used.
Later the Http::One::Server::handleReply member updates again
the same ALE member without checking if it is already set.

This is a Measurement Factory project
@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Jun 25, 2019
@rousskov rousskov changed the title Bug 4842: Memory leak when external_acl is used with http_reply_access Bug 4842: Memory leak when http_reply_access uses external_acl Jun 25, 2019
@rousskov rousskov requested a review from yadij June 25, 2019 17:23
@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Jun 25, 2019
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change looks correct to me. Thank you for checking other, similar ALE reply reset cases.

I am surprised this leak has been hiding for almost four years since fbbea66, so I have asked @yadij to double check that attribution.

Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I expect it has remained hidden for so long because http_reply_access is not a commonly used directive, and use of external ACL in it even less so.

@yadij yadij added the M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels label Jun 29, 2019
squid-anubis pushed a commit that referenced this pull request Jun 29, 2019
Http::One::Server::handleReply() sets AccessLogEntry::reply which may
already be set. It is already set, for example, when the ACL code 
has already called syncAle() because external ACLs require an ALE.

This bug was introduced by commit fbbea66.

This is a Measurement Factory project.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Jun 29, 2019
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Jun 29, 2019
squidadm pushed a commit to squidadm/squid that referenced this pull request Jul 4, 2019
…-cache#424)

Http::One::Server::handleReply() sets AccessLogEntry::reply which may
already be set. It is already set, for example, when the ACL code
has already called syncAle() because external ACLs require an ALE.

This bug was introduced by commit fbbea66.

This is a Measurement Factory project.
yadij pushed a commit that referenced this pull request Jul 4, 2019
Http::One::Server::handleReply() sets AccessLogEntry::reply which may
already be set. It is already set, for example, when the ACL code
has already called syncAle() because external ACLs require an ALE.

This bug was introduced by commit fbbea66.

This is a Measurement Factory project.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants