CI: enhance FreeBSD staging tests#2372
Open
kinkie wants to merge 30 commits intosquid-cache:masterfrom
Open
Conversation
Contributor
Author
|
These workflows have been tested at |
rousskov
requested changes
Feb 9, 2026
rousskov
added
the
S-waiting-for-author
kinkie
changed the title
Contributor
Author
|
Test-run of the updated workflow: https://github.com/kinkie/squid/actions/runs/21858341109 |
kinkie
added
S-waiting-for-reviewer
rousskov
requested changes
Feb 10, 2026
Contributor
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Thank you for advancing this PR.
rousskov
added
S-waiting-for-author
Fail Fast on FreeBSD Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
squid-anubis
added
the
M-failed-description
Collaborator
|
Cannot create a git commit message from PR title and description. Error while parsing PR description body: Problematic parser input: Please see PR title and description formatting requirements for more details. This message was added by Anubis bot. Anubis will add a new message if the error text changes. Anubis will remove M-failed-description label when there are no corresponding failures to report. |
…e#2374) In this context, escaping escaped URI always produces incorrect URI because `%` character in the escaped URI gets escaped again. Feeding the result of the first rfc1738_escape() call to the second call is also dangerously wrong because the result of the first call gets invalidated during the second call. No other cases of such "chained" rfc1738_escape() calls were found. Broken since 2002 commit e6ccf24.
…-cache#2373) Squid detects SQUID_X509_V_ERR_DOMAIN_MISMATCH errors during various processing stages, including when receiving an HTTP request on a successfully bumped TLS connection. If that request targets a domain not covered by the server certificate, and sslproxy_cert_error prohibits a mismatch (it does by default), then Squid terminates the transaction with an ERR_SECURE_CONNECT_FAIL response. That generated error response body lacked %x and %D error details: ```diff The system returned: - [No Error] (TLS code: [Unknown Error Code]) + [No Error] (TLS code: SQUID_X509_V_ERR_DOMAIN_MISMATCH) - [No Error Detail] + Certificate does not match domainname: /L=.../O=.../CN=example.com ``` The first `[No Error]` expansion of %E remains unchanged because this particular error does not set `errno`. ConnStateData::serveDelayedError() changes fix the above problem but %x expansion in error pages and %err_detail in access log get a misleading `+broken_cert` detail. To address that flaw, we changed the default for broken certificate in Security::ErrorDetail constructor API from peer certificate to nil. When broken certificate is nil, ErrorDetail now uses valid certificate to expand %ssl_cn and similar certificate-inspecting error page %codes. All Security::ErrorDetail creators were checked and adjusted if needed: * ConnStateData::serveDelayedError(): No caller changes. Using the new ErrorDetail creation API fixes this code by supplying nil broken certificate (because the certificate is _valid_ in this context). * ssl_verify_cb(): No caller changes. We already use peer certificate as the default broken certificate because doing so is "reasonable" here. * Security::PeerConnector::sslCrtvdCheckForErrors(): Adjusted to keep the original "if there was no error_cert_ID, then use peerCert" behavior while using new Security::ErrorDetail creation API. Thus, the last two contexts are not affected by this error reporting API change. The exceptional serveDelayedError() caller is affected, but Squid did not report any certificate detail in that case until this branch fixes, so this branch does not change one "reporting certificate" to another; it only starts reporting (important) information when none was available before. This is a Measurement Factory project.
Fix handling of malformed ICP queries and replies instead of passing invalid URL pointer to consumers, leading to out-of-bounds memory reads and other problems. These fixes affect both ICP v2 and ICP v3 traffic. * Reject packets with URLs that are not NUL-terminated. * Reject packets with URLs containing embedded NULs or trailing garbage. The above two restrictions may backfire if popular ICP agents do send such malformed URLs, and we will need to do more to handle them correctly, but it is _safe_ to reject them for now. Also protect icpHandleUdp() from dereferencing a nil icpOutgoingConn pointer. It is not clear whether icpHandleUdp() can be exposed to nil icpOutgoingConn in current code. More work is needed to polish this.
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.
Update Swedish (sv) translations for error pages. Changes: - Removed fuzzy flags from corrected translations - Fixed translation for secure connection error messages - Updated PO-Revision-Date to 2026-02-20 Translator: Daniel Nylander <daniel@danielnylander.se>
The current official GPLv2 text has been modified to remove the FSF street address. Bring our GPL copy up to date with the official GPLv2 license document. Also, libltdl license has similar changes. Update our CREDITS file to match the v2.5.3+ libltdl/ldtl.h import. There are no legal implications from this change.
squid-anubis
removed
the
M-failed-description
…reebsd-staging-test
kinkie
added
S-waiting-for-reviewer
Avoid noisy out-of-scope changes. My change request was specific to "runs-on": squid-cache#2372 (comment) This reverts commit 1ea3858.
GitHub says this quoting is "required" for "expressions" like ours: https://docs.github.com/en/actions/how-tos/write-workflows/choose-where-workflows-run/choose-the-runner-for-a-job Context: squid-cache#2372 (comment)
And polish its wording/formatting a bit.
This change preserves that step key. We probably want to add github.job to other ccache step keys because the YAML job name (e.g., "freebsd" in this case) may have a huge effect on what is being cached. In other words, we should not reuse ccache across jobs.
It is not used in the other "prepare" action. It is GitHub default on runners using "sh" and "bash", and it is certainly GitHub intent to enable "fast-fail" whenever possible: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#exit-codes-and-error-action-preference Hopefully, it is not needed here. If it is needed, we will document that need explicitly.
PR description claims that we want to maximize "signal-to-execution-time ratio" and that "the main purpose of freebsd tests is to quickly identify linux-isms and portability issues, not internal code dependencies and misalignments". Both goals are accomplished by testing using layer-02-maximus alone. Also, we use the same "just use layer-02-maximus" approach for * cov-build in coverity-scan.yaml * CodeQL-tests in quick.yaml TODO: Apply the same principles to all non-Linux tests.
rousskov
requested changes
Mar 12, 2026
Contributor
There was a problem hiding this comment.
I fixed PR description to remove a stale reference to OpenBSD, improve wording, and to sync with current branch state. Please check.
I addressed a few old change requests and pushed a few additional corrections. Please check.
I think we are almost done here! There is only one change request remaining AFAICT.
| nettle \ | ||
| pkgconf | ||
| pkgconf \ | ||
| portmaster |
Contributor
There was a problem hiding this comment.
Do we have to add portmaster? PR description says that we no longer rely on ports, so I hope we do not need this dependency (anymore).
rousskov
added
S-waiting-for-author
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
7 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.
Cover all FreeBSD versions supported by the FreeBSD Project.
Fully rely on packages, not ports, solving the "version mismatch"
problem that had led us to remove FreeBSD 13.5 in commit 99fca3a.
Enable ccache for faster build times.
Restrict testing to the maximus test layer to maximize
signal-to-execution-time ratio: The main purpose of FreeBSD tests is to
quickly identify Linux-isms and portability issues, not internal code
dependencies and misalignments.
Prepare to also support arm64 and riscv64, but leave them disabled for
now as they are respectively too slow
and broken at this time.