out_stackdriver: fix memory leaks in pthread resources and mutex #11330
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds initialization-tracking flags and explicit cleanup for the Stackdriver plugin: thread-local OAuth2 keys are deleted on exit and the token mutex is destroyed during configuration teardown. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| oauth2_cache_cleanup(); | ||
| flb_stackdriver_conf_destroy(ctx); |
There was a problem hiding this comment.
On shutdown cb_stackdriver_exit now unconditionally calls oauth2_cache_cleanup(), but cb_stackdriver_init returns before oauth2_cache_init() when test_log_entry_format is set or when initialization fails before that block (see lines 1234-1239). In those cases the global pthread keys remain uninitialized, so pthread_key_delete here operates on invalid keys, which is undefined and can raise EINVAL or tear down keys needed by another Stackdriver instance still running.
Useful? React with 👍 / 👎.
| flb_kv_release(&ctx->config_labels); | ||
| flb_kv_release(&ctx->resource_labels_kvs); | ||
| pthread_mutex_destroy(&ctx->token_mutex); |
There was a problem hiding this comment.
flb_stackdriver_conf_destroy now always calls pthread_mutex_destroy(&ctx->token_mutex) even when the mutex was never initialized: cb_stackdriver_init returns early for test_log_entry_format before pthread_mutex_init (lines 1234-1250), and many configuration error paths invoke this destroy helper during partial setup. Destroying an uninitialized mutex is undefined and can trigger runtime errors during these early-return or failure scenarios.
Useful? React with 👍 / 👎.
Example Configuration File[SERVICE]
Flush 1
Log_Level info
Daemon off
[INPUT]
Name dummy
Tag test.data
Dummy {"message": "test log entry"}
[OUTPUT]
Name stackdriver
Match *
project_id_key logging.googleapis.com/projectId
# In production, add:
# google_service_credentials /path/to/service-account.json
# or
# metadata_server http://metadata.google.internalDebug Log Output from TestingValgrind Outputvalgrind --leak-check=full --show-leak-kinds=all \
--track-origins=yes \
--error-exitcode=1 \
./bin/flb-rt-out_stackdriverLeak Summary: Result: ✅ No definite or indirect memory leaks detected.
Testing Summary
Change SummaryThis fix adds proper cleanup for thread-local storage and synchronization resources: Before Fix:
After Fix:
Additional Notes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/out_stackdriver/stackdriver.cplugins/out_stackdriver/stackdriver_conf.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T18:45:02.283Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 11286
File: src/flb_http_client.c:1657-1715
Timestamp: 2025-12-17T18:45:02.283Z
Learning: In the Fluent Bit OAuth2 implementation (src/flb_oauth2.c), the function flb_oauth2_get_access_token returns a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation. The OAuth2 context manages the token lifecycle, so callers must not free the returned token pointer.
Applied to files:
plugins/out_stackdriver/stackdriver.cplugins/out_stackdriver/stackdriver_conf.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
🔇 Additional comments (1)
plugins/out_stackdriver/stackdriver.c (1)
3097-3097: Proper placement of pthread key cleanup.The call to
oauth2_cache_cleanup()is correctly placed in the exit callback before destroying the context. The NULL check onctxat lines 3093-3095 provides some protection against improper invocation.
|
|
||
| flb_kv_release(&ctx->config_labels); | ||
| flb_kv_release(&ctx->resource_labels_kvs); | ||
| pthread_mutex_destroy(&ctx->token_mutex); |
There was a problem hiding this comment.
Critical: Potential undefined behavior from destroying uninitialized mutex.
The token_mutex is only initialized in cb_stackdriver_init() (stackdriver.c:1250), but flb_stackdriver_conf_destroy() can be called in scenarios where the mutex was never initialized:
- Test mode early return: When
test_log_entry_formatis true,cb_stackdriver_init()returns at line 1238 before initializing the mutex at line 1250 - Initialization failure: If
cb_stackdriver_init()fails before reaching line 1250, the mutex remains uninitialized
Destroying an uninitialized mutex invokes undefined behavior per POSIX.
🔎 Proposed fix: Track mutex initialization state
Add a flag to track whether the mutex was initialized:
In stackdriver.h, add to the context structure:
+ int token_mutex_initialized;In stackdriver.c at line 1250:
/* Create mutex for acquiring oauth tokens (they are shared across flush coroutines) */
pthread_mutex_init(&ctx->token_mutex, NULL);
+ ctx->token_mutex_initialized = FLB_TRUE;In stackdriver_conf.c at line 709:
flb_kv_release(&ctx->config_labels);
flb_kv_release(&ctx->resource_labels_kvs);
- pthread_mutex_destroy(&ctx->token_mutex);
+ if (ctx->token_mutex_initialized) {
+ pthread_mutex_destroy(&ctx->token_mutex);
+ }
flb_free(ctx);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugins/out_stackdriver/stackdriver_conf.c around line 709, destroying
ctx->token_mutex unconditionally can invoke undefined behavior if the mutex was
never initialized; add a boolean flag (e.g., token_mutex_initialized) to the
context struct in stackdriver.h, set that flag to true immediately after
successfully initializing the mutex in cb_stackdriver_init() (stackdriver.c
around line 1250) and set it false on failures/early returns, and then in
flb_stackdriver_conf_destroy() (plugins/out_stackdriver/stackdriver_conf.c line
~709) check the flag before calling pthread_mutex_destroy(&ctx->token_mutex) and
only destroy when initialized.
Addressed Code Review FeedbackUpdated the fix to properly handle initialization state and prevent cleanup of uninitialized resources: Changes Made
Safety Guarantees✅ Prevents deletion of uninitialized pthread keys when Testing
SummaryThe original fix addressed memory leaks but had potential for undefined behavior in edge cases. This update adds proper initialization tracking to ensure cleanup operations are always safe. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/out_stackdriver/stackdriver.c (1)
1253-1254: Consider checking pthread_mutex_init() return value before setting the flag.Line 1253 calls
pthread_mutex_init()but doesn't check its return value. If initialization fails (e.g., ENOMEM), settingtoken_mutex_initialized = FLB_TRUEat line 1254 would be incorrect and could lead topthread_mutex_destroy()being called on an uninitialized mutex during cleanup.🔎 Proposed fix to add error handling
/* Create mutex for acquiring oauth tokens (they are shared across flush coroutines) */ -pthread_mutex_init(&ctx->token_mutex, NULL); -ctx->token_mutex_initialized = FLB_TRUE; +ret = pthread_mutex_init(&ctx->token_mutex, NULL); +if (ret != 0) { + flb_plg_error(ctx->ins, "failed to initialize token_mutex"); + return -1; +} +ctx->token_mutex_initialized = FLB_TRUE;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/out_stackdriver/stackdriver.cplugins/out_stackdriver/stackdriver.hplugins/out_stackdriver/stackdriver_conf.c
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_stackdriver/stackdriver_conf.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T18:45:02.283Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 11286
File: src/flb_http_client.c:1657-1715
Timestamp: 2025-12-17T18:45:02.283Z
Learning: In the Fluent Bit OAuth2 implementation (src/flb_oauth2.c), the function flb_oauth2_get_access_token returns a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation. The OAuth2 context manages the token lifecycle, so callers must not free the returned token pointer.
Applied to files:
plugins/out_stackdriver/stackdriver.hplugins/out_stackdriver/stackdriver.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-without-cxx (3.31.6)
🔇 Additional comments (4)
plugins/out_stackdriver/stackdriver.c (3)
75-82: LGTM! Proper guard prevents cleanup of uninitialized pthread keys.The
oauth2_cache_initializedflag correctly preventspthread_key_delete()from being called on uninitialized keys. This resolves the undefined behavior that occurred whentest_log_entry_formatcaused an early return beforeoauth2_cache_init()was called.
1249-1250: LGTM! Proper flag initialization pattern.Setting
oauth2_cache_initialized = FLB_TRUEimmediately afteroauth2_cache_init()ensures the cleanup guard works correctly.
3101-3102: LGTM! Proper cleanup order.Calling
oauth2_cache_cleanup()beforeflb_stackdriver_conf_destroy()ensures pthread keys are deleted before the context is destroyed. The internal flag check inoauth2_cache_cleanup()handles the case where initialization was skipped.plugins/out_stackdriver/stackdriver.h (1)
207-208: No changes needed—flags are properly initialized.The
token_mutex_initializedandoauth2_cache_initializedflags are safely initialized. The context is allocated viaflb_calloc(1, sizeof(struct flb_stackdriver)), which both allocates memory AND zeroes it (equivalent tocalloc), ensuring these int flags start as 0 (FLB_FALSE). They are then explicitly set to FLB_TRUE only after their respective initialization functions succeed. The cleanup functions correctly guard against uninitialized resources by checking these flags before invokingpthread_mutex_destroy()andpthread_key_delete(). This is a sound defensive programming pattern with no undefined behavior risk.Likely an incorrect or invalid review comment.
Cleaned up commit history and created new feature branchThe previous commits to master made the history messy. I've created a clean feature branch with both fixes: New Branch: This branch contains the same two commits in correct order:
What this fixes:
Next Steps:
The messy commits on master can be cleaned up later or ignored - this feature branch contains the complete, clean implementation. |
3 participants
Fixed critical memory leaks in Stackdriver output plugin where thread-specific storage and synchronization resources were never released during plugin lifecycle.
The 3 pthread keys (oauth2_type, oauth2_token, oauth2_token_expires) created in oauth2_cache_init() were never deleted with pthread_key_delete().
The mutex (token_mutex) created in cb_stackdriver_init() was never destroyed with pthread_mutex_destroy().
Each worker thread accumulates leaked resources. With multiple workers or frequent configuration reloads, memory usage grows unbounded.
Added oauth2_cache_cleanup() function to delete all pthread keys and call it in cb_stackdriver_exit() before context cleanup. Added pthread_mutex_destroy() in flb_stackdriver_conf_destroy().
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.