coverity cid 1461163 #1838
Closed
coverity cid 1461163 #1838
Conversation
This file contains 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
Contributor
kinkie
commented
Jun 9, 2024
kinkie
commented
- ext_time_quota_acl: convert logging to c++
- CI: Build layers independently to speed up GitHub Actions tests (CI: Build layers independently to speed up GitHub Actions tests #1787)
- CI: Add clang to GitHub Actions build-tests matrix (CI: Add clang to GitHub Actions build-tests matrix #1812)
- Ensure ACLChecklist::markFinished() is called (Ensure ACLChecklist::markFinished() is called #1809)
- Fix build with clang v18 [-Wvla-cxx-extension] (Fix build with clang v18 [-Wvla-cxx-extension] #1813)
- Docs: REQUIRED in ident_regex, proxy_auth_regex, ext_user_regex (Docs: REQUIRED in ident_regex, proxy_auth_regex, ext_user_regex #1818)
- testRandomUuid: use cppunit exception tests (testRandomUuid: use cppunit exception tests #1814)
- testCacheManager: use cppunit exception tests (testCacheManager: use cppunit exception tests #1811)
- testHttpRange: use cppunit framework (testHttpRange: use cppunit framework #1816)
- MinGW: fix error: cannot convert 'size_t' to 'int' (MinGW: fix error: cannot convert 'size_t*' to 'int*' #1822)**
- MinGW: Fix error: cannot convert 'const int' to 'const char' (MinGW: Fix error: cannot convert 'const int*' to 'const char*' #1824)**
- annotate_client and annotate_transaction ACLs must always match (annotate_client and annotate_transaction ACLs must always match #1820)
- MinGW: Fix error: cannot convert 'size_t' to 'int' (MinGW: Fix error: cannot convert 'size_t*' to 'int*' #1825)**
- Remove Ident protocol support (Remove Ident protocol support #1827)
- Bug 5378: type mismatch in libTrie (Bug 5378: type mismatch in libTrie #1830)
- Improve ErrorState debugging (Improve ErrorState debugging #1768)
- Do not die when parsing obsolete log_access and log_icap (Do not die when parsing obsolete log_access and log_icap #1832)
- Maintenance: Remove debug_log wrapper (Maintenance: Remove debug_log wrapper #1833)
Inspired by Coverity CID 1461163, "Invalid type in argument to printf format specifier" Rely on c++ ostream type handling for proper handling of integer values
…d-cache#1787) Adding a second matrix test dimension can decrease testing wall time by about 75% by parallelizing tests
Add "compiler" dimension to build-tests matrix. Do not test layer-04-noauth-everything to partially compensate for the new matrix dimension repeating other layer tests 3 times. Change build-tests artifacts name pattern to include compiler dimension to avoid name clashes that would result in logs being overwritten and lost. Also use target environment instead of runner environment in anticipation of adding container-based tests where the two differ.
For example, ACLChecklist::resumeNonBlockingCheck() missed a markFinished() call when prepNonBlocking() returned false after a reconfiguration. Missing markFinished() calls result in the last evaluated ACL name being unset in nonBlockingCheck() callbacks.
src/fs/rock/RockRebuild.cc:356:17: error: variable length arrays
in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
char hdrBuf[SwapDir::HeaderSize];
note: initializer of 'HeaderSize' is unknown
…d-cache#1818) The three ACLs were documented as matching any username when configured with a parameter spelled "REQUIRED". Neither actually treated that parameter specially -- all interpreted it as an ordinary regex. This dangerous documentation bug was introduced in 2000 commit 145cf92 that added ident_regex and proxy_auth_regex support. It was then duplicated in 2003 commit abb929f that added ext_user_regex support. This minimal documentation fix does not imply that these ACLs should not treat REQUIRED values specially. Enabling such special treatment requires significant code changes, especially if we want to do that well and without duplicating the corresponding code.
Do not hand-roll tests for exception-throwing code
Do not hand-roll tests for exception-throwing code
Resolve a long-standing TODO and stop hand-rolling HTTP range unit tests
Remove struct addrinfo as we do.
…d-cache#1820) WARNING: markAsTunneled ACL is used in context without an HTTP request. Assuming mismatch. Our annotate_client and annotate_transaction ACLs are documented as "always matching", and some existing Squid configurations rely on that invariant. Both ACLs did not match when they lacked access to the current transaction information because their requiresRequest() method returned true; annotate_client ACL also did not match after the client-to-Squid connection was gone and when the transaction was not associated with a client-to-Squid connection. This change makes ACL code conform to documentation. Squid still warns the admin if an ACL cannot annotate. Such warnings may indicate s Squid bug or misconfiguration, but mismatching in those cases causes more harm because it makes it impossible for the admin to rely on the primary matching invariant. "One reliable invariant plus one unreliable side effect" is the lesser evil than "two unreliable effects". # the following must deny even if it cannot annotate http_access deny markAsDenied # the following might not log denied traffic (with a prior warning) access_log syslog:daemon.err markedAsDenied Also fixed annotate_client to annotate the current transaction even after ConnStateData destruction. Such annotations may happen when, for example, Squid continues a large download after the HTTP client is gone.
Removing another addrinfo as we do.
Ident protocol (RFC 931 obsoleted by RFC 1413) has been considered seriously insecure and broken since at least 2009 when SANS issued an update recommending its removal from all networks. Squid Ident implementation suffered from assertions (since 2021 commit e227da8) and memory leaks (since 2015 commit fbbea66). Ident implementation design increased ACLChecklist::goAsync() design complexity. An external ACL helper can be written to perform Ident transactions. Configurations using ident/ident_regex ACLs, %ui logformat codes, %IDENT external_acl_type format code, or ident_lookup_access/ident_timeout directives are now rejected, leading to fatal startup failures: FATAL: Invalid ACL type 'ident_regex' FATAL: Invalid ACL type 'ident' ERROR: configuration failure: Unsupported %code: '%ui' ERROR: configuration failure: Unsupported %code: '%IDENT' squid.conf(6): unrecognized: 'ident_lookup_access' squid.conf(7): unrecognized: 'ident_timeout' To avoid inconveniencing admins that do _not_ use Ident features, access logs with "common" and "combined" logformats now always receive a dash in the position of what used to be a %ui record field. Co-authored-by: Amos Jeffries <[email protected]>
TrieNode::add() incorrectly computed an offset of an internal data structure, resulting in out-of-bounds memory accesses that could cause corruption or crashes. This bug was discovered and detailed by Joshua Rogers at https://megamansec.github.io/Squid-Security-Audit/esi-underflow.html where it was filed as "Buffer Underflow in ESI".
In triage, it is often useful to know that ErrorState was created _when_ it was created rather than waiting for errorAppendEntry() or errorSend() debugging, especially if those error handling stages are not reached. Also report HTTP status code of the error response (when available).
…#1832) All obsolete directives except for log_access and log_icap do not result in Squid instance death today. Treating those two directives specially is not necessary but does complicate reconfiguration improvements a bit. Future refactoring might add support for treating (some or all) obsolete directives as fatal configuration errors, but we can simplify for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.