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

V6 backports #1986

Merged
merged 3 commits into from
Jan 31, 2025
Merged

V6 backports #1986

merged 3 commits into from
Jan 31, 2025

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented Jan 20, 2025

Backporting commits:

  • 8b31858 : Portability: remove explicit check for libdl
  • dd3292c : Refactor peerRefreshDNS() to clarify its (void*)1 logic
  • 0ef767a: Nil request dereference in
    ACLExtUser and SourceDomainCheck ACLs

OpenBSD does not have libdl, as it has dlopen() in libc.
It is not really needed, and force-requiring the presence of libdl
causes ./configure to fail on openbsd:

    checking for dlopen in -ldl... no
    configure: error: Required library 'dl' not found
@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Jan 20, 2025
src/acl/SourceDomain.cc Outdated Show resolved Hide resolved
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Jan 21, 2025
src/neighbors.cc Outdated Show resolved Hide resolved
@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Jan 27, 2025
@kinkie kinkie added S-waiting-for-more-reviewers needs a reviewer and/or a second opinion and removed S-waiting-for-author author action is expected (and usually required) labels Jan 27, 2025
@yadij
Copy link
Contributor

yadij commented Jan 28, 2025

@kinkie, this being a backport of multiple changes it is destined for a rebase-merge instead of a squash-merge. That means edits like these review changes and build fixes need to be manually combined in the PR branch.

if you were not aware how to do that:

  • In your branch checkout run:
    • git rebase -i v6
  • In the editor that comes up:
    • reorder the fixes underneath the commit they change
    • replace action (first character) p with f

N.B. Ideally manual backports are separate PRs to avoid this type of tricks.

@yadij yadij added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-more-reviewers needs a reviewer and/or a second opinion labels Jan 28, 2025
…1950)

Creating a raw pointer with 1 as an address/value raised red flags, and
it was difficult to interpret tricky peerRefreshDNS() logic correctly.
…id-cache#1931)

ACLExtUser-based ACLs (i.e. ext_user and ext_user_regex) dereferenced a
nil request pointer when they were used in a context without a request
(e.g., when honoring on_unsupported_protocol).

SourceDomainCheck-based ACLs (i.e. srcdomain and srcdom_regex) have a
similar bug, although we do not know whether broken slow ACL code is
reachable without a request (e.g., on_unsupported_protocol tests cannot
reach that code until that directive starts supporting slow ACLs). This
change does not start to require request presence for these two ACLs to
avoid breaking any existing configurations that "work" without one.
@kinkie
Copy link
Contributor Author

kinkie commented Jan 28, 2025

@kinkie, this being a backport of multiple changes it is destined for a rebase-merge instead of a squash-merge. That means edits like these review changes and build fixes need to be manually combined in the PR branch.

if you were not aware how to do that:

  • In your branch checkout run:

    • git rebase -i v6
  • In the editor that comes up:

    • reorder the fixes underneath the commit they change
    • replace action (first character) p with f

N.B. Ideally manual backports are separate PRs to avoid this type of tricks.

Thanks for the advice, and yes I'll do individual backports next time

@kinkie kinkie added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Jan 28, 2025
squid-anubis pushed a commit that referenced this pull request Jan 30, 2025
Backporting commits:

* 8b31858 :  Portability: remove explicit check for libdl
* dd3292c : Refactor peerRefreshDNS() to clarify its (void*)1 logic
* 0ef767a: Nil request dereference in
           ACLExtUser and SourceDomainCheck ACLs
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-failed-staging-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Jan 30, 2025
@yadij yadij merged commit 33670ba into squid-cache:v6 Jan 31, 2025
9 checks passed
@yadij yadij added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box M-failed-staging-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jan 31, 2025
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.

5 participants