Fix ASAN-detected plugin bugs across five components#13013
Open
bryancall wants to merge 6 commits intoapache:masterfrom
Open
Fix ASAN-detected plugin bugs across five components#13013bryancall wants to merge 6 commits intoapache:masterfrom
bryancall wants to merge 6 commits intoapache:masterfrom
Conversation
Use sized format specifier %.*s with string_view to avoid reading past the end of the buffer.
Cast to JA4_data* instead of std::string* before delete to match the actual allocated type and invoke the correct destructor.
Move _arena declaration before _roots so the arena outlives the directives that reference its memory. C++ destroys members in reverse declaration order, so _arena must be declared first.
Free urlstr before early return on invalid content length. The string was allocated by TSHttpTxnEffectiveUrlStringGet but leaked on the error path.
Save writefd value before freeing its backing memory with OPENSSL_free, then use the saved value for close() and the debug message.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple ASAN-reported memory-safety defects across several ATS plugins and test-plugin code paths, focusing on targeted fixes to prevent invalid reads, type-mismatched deletes, leaks, and lifetime issues.
Changes:
- Fix a use-after-free in the OpenSSL async engine test plugin cleanup path by not dereferencing freed memory.
- Fix an over-read / non-NUL-terminated string logging issue in
stats_over_httpdebug output by using length-bounded formatting. - Fix a leak on early-return in the
sliceplugin by freeing the transaction URL string before returning. - Adjust
txn_boxmember destruction order to ensure directive-owned memory arenas outlive directive trees. - Update
ja4_fingerprintVConn close cleanup to delete the correct stored type.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/tools/plugins/async_engine.c | Avoids dereferencing freed memory during async wait context cleanup. |
| plugins/stats_over_http/stats_over_http.cc | Makes debug logging safe for non-null-terminated path suffix views. |
| plugins/slice/server.cc | Frees TSHttpTxnEffectiveUrlStringGet result on early return to avoid leaks. |
| plugins/experimental/txn_box/plugin/include/txn_box/Config.h | Ensures arena outlives directive roots by reordering members for correct destruction. |
| plugins/experimental/ja4_fingerprint/plugin.cc | Changes cleanup delete to match the actual object type stored in the vconn user arg. |
TSUserArgSet stores a JA4_data* but handle_read_request_hdr retrieved it as std::string*. This worked by accident since fingerprint is the first member, but is technically undefined behavior. Cast to JA4_data* consistently and access the fingerprint member explicitly.
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
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.
Summary
stats_over_http,ja4_fingerprint,txn_box,slice, andasync_enginewith targeted correctness fixes.Test plan