Open
Conversation
The results array allocated via malloc() in CB_context_dump was never freed after iterating over the context names.
The ConfigCache class stores raw S3Config pointers in its _cache map but lacks a destructor, leaking all configs on shutdown. Add a destructor that deletes all remaining S3Config pointers.
The global g_config linked list of HCFileInfo structs (and their ok/miss buffers and HCFileData) were never freed. Add a shutdown handler via TS_LIFECYCLE_SHUTDOWN_HOOK to walk the list and free all allocations when the plugin shuts down.
The ConfigInfo destructor frees body_data and body_data_mutex but never frees log_info.filename, which is allocated via strdup() when the -d option is parsed. This adds a free() call in the destructor, guarded by a check against the default static PLUGIN_TAG pointer.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses several leak reports found via ASAN/LSAN runs by adding missing cleanup paths across multiple ATS plugins and examples.
Changes:
- Free the results array in
client_context_dumpafterTSSslClientContextsNamesGet(). - Add shutdown-time cleanup for
healthchecksglobal config list. - Add
ConfigCachedestructor cleanup inorigin_server_authand freestale_response’s dynamically allocated log filename.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
plugins/origin_server_auth/origin_server_auth.cc |
Adds ConfigCache destructor to delete cached S3Config objects. |
plugins/healthchecks/healthchecks.cc |
Adds shutdown hook and helper to free the HCFileInfo linked list. |
plugins/experimental/stale_response/stale_response.h |
Frees log_info.filename when it is dynamically allocated. |
example/plugins/c-api/client_context_dump/client_context_dump.cc |
Frees the caller-allocated results array. |
…own hook frees g_config while hc_thread is still running its infinite inotify loop, causing a use-after-free. Since this leak only occurs at process exit, let the OS reclaim the memory.
Contributor
Author
|
CI: Fedora failure is unrelated — /retest |
Contributor
Author
|
[approve ci fedora] |
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
client_context_dump— missingfree(results)afterTSSslClientContextsNamesGetorigin_server_auth— addConfigCachedestructor to delete cachedS3Configobjectsstale_response— freelog_info.filenamewhen not the static defaultFound via ASAN-enabled autest runs on Fedora 43.
Test plan
ENABLE_ASAN=ONand ran full autest suite — no new LSAN reports for these symbols