out_stackdriver: fix multiple memory leaks and potential corruption#11879
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefactors GCE metadata fetching to return results via an output ChangesStackdriver Plugin Memory and Parameter Corrections
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/out_stackdriver/gce_metadata.c`:
- Around line 99-101: The code currently sets ret_code = 0 before calling
flb_sds_copy and does not check its return; change the flow so you call
flb_sds_copy(*payload, c->resp.payload, c->resp.payload_size), check the result
for NULL (or failure) and only set ret_code = 0 on success, otherwise set an
appropriate error code and handle cleanup; specifically update the block around
flb_sds_copy, *payload, ret_code and c->resp.payload/c->resp.payload_size to
validate the copy result and avoid returning success when allocation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fe82681-9af8-4217-8935-f7003af0c3ff
📒 Files selected for processing (3)
plugins/out_stackdriver/gce_metadata.cplugins/out_stackdriver/stackdriver.cplugins/out_stackdriver/stackdriver_conf.c
![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.
Reviewed commit: f7b2b437c1
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_stackdriver/gce_metadata.c (1)
47-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOverwriting
*payloadleaks the original buffer whenflb_sds_cat/flb_sds_copyfails.
flb_sds_catandflb_sds_copyreturnNULLon a (re)allocation failure but do not free the original buffer. Assigning the result directly back to*payloadoverwrites the only reference to that buffer, so it leaks; the caller's subsequentflb_sds_destroy(payload)then operates onNULL(no-op). This mirrors the leak class this PR is fixing, and the rest of this file already uses the temp-variable pattern (see lines 143-150) to avoid it.This applies to all four sites: lines 47, 54, 61 and 99.
🛡️ Proposed fix using a temp variable
int ret; int ret_code; size_t b_sent; + flb_sds_t tmp; struct flb_connection *metadata_conn; struct flb_http_client *c; /* If runtime test mode is enabled, add test data */ if (ctx->ins->test_mode == FLB_TRUE) { if (strcmp(uri, FLB_STD_METADATA_PROJECT_ID_URI) == 0) { - *payload = flb_sds_cat(*payload, "fluent-bit-test", 15); - if (!*payload) { + tmp = flb_sds_cat(*payload, "fluent-bit-test", 15); + if (!tmp) { return -1; } + *payload = tmp; return 0; }Apply the same temp-variable pattern to the zone (line 54), instance-id (line 61), and HTTP-200 (line 99) assignments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/out_stackdriver/gce_metadata.c` around lines 47 - 105, The code assigns the return of flb_sds_cat and flb_sds_copy directly into *payload which leaks the original SDS when the call returns NULL; change those four sites (the branches handling FLB_STD_METADATA_PROJECT_URI, FLB_STD_METADATA_ZONE_URI, FLB_STD_METADATA_INSTANCE_ID_URI and the HTTP-200 path where flb_sds_copy is used) to use a temporary flb_sds_t variable (e.g., tmp) to receive the result, check tmp for NULL, only assign *payload = tmp on success, otherwise preserve the original *payload and set the error/ret_code accordingly (mirror the temp-variable pattern already used around lines 143-150).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@plugins/out_stackdriver/gce_metadata.c`:
- Around line 47-105: The code assigns the return of flb_sds_cat and
flb_sds_copy directly into *payload which leaks the original SDS when the call
returns NULL; change those four sites (the branches handling
FLB_STD_METADATA_PROJECT_URI, FLB_STD_METADATA_ZONE_URI,
FLB_STD_METADATA_INSTANCE_ID_URI and the HTTP-200 path where flb_sds_copy is
used) to use a temporary flb_sds_t variable (e.g., tmp) to receive the result,
check tmp for NULL, only assign *payload = tmp on success, otherwise preserve
the original *payload and set the error/ret_code accordingly (mirror the
temp-variable pattern already used around lines 143-150).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a45d471-7522-4c32-a796-f0124a1c587a
📒 Files selected for processing (3)
plugins/out_stackdriver/gce_metadata.cplugins/out_stackdriver/stackdriver.cplugins/out_stackdriver/stackdriver_conf.c
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/out_stackdriver/stackdriver_conf.c
- plugins/out_stackdriver/stackdriver.c
- Update fetch_metadata to use flb_sds_t * to handle sds reallocation correctly - Use temporary variable in fetch_metadata to avoid leaking original SDS on failure - Fix error handling in fetch_metadata when flb_sds_copy fails - Fix memory leaks in stackdriver.c by destroying rval and http_request in failure paths - Fix memory leak in stackdriver_conf.c by destroying project_id before overwriting it Signed-off-by: Tim Bai <timbai@google.com>
|
/lgtm |
|
Hi edsiper, can you please help merge the change? Thanks! |
4 participants
This pull request addresses several memory management issues in the Stackdriver output plugin, fixing potential memory leaks and invalid pointer usage due to SDS string reallocation.
Changes:
plugins/out_stackdriver/gce_metadata.c
fetch_metadatafunction signature to accept a pointer toflb_sds_t(flb_sds_t *payload). This ensures that if the SDS string is reallocated duringflb_sds_catorflb_sds_copy, the caller's pointer is correctly updated, preventing the use of invalid pointers.fetch_metadatato pass the address of the payload.flb_sds_catandflb_sds_copyresults back to the payload pointer, including error checks.plugins/out_stackdriver/stackdriver.c
pack_resource_labels: Added missingflb_ra_key_value_destroy(rval)when the extracted value object is valid but not of string type, fixing a memory leak.stackdriver_format: Added missingdestroy_http_request(&http_request)in an early return path (error case) to prevent leaking resources associated with the HTTP request.plugins/out_stackdriver/stackdriver_conf.c
read_credentials_file: Added cleanup forctx->project_idbefore it gets overwritten if it was already initialized, preventing a memory leak if duplicate keys exist in the configuration.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