sds: general fixes for sds_cat() usage - #11805
Merged
Merged
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 (11)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughProtects meminfo metric-name construction by using safe SDS concatenation and explicit null/return checks, with proper cleanup of temporaries and lists on allocation/concat failure. ChangesNode Exporter meminfo safe concatenation
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/in_node_exporter_metrics/ne_meminfo_linux.c (1)
260-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd return value checks to
flb_sds_cat_safecalls inmeminfo_update.Lines 261-262 do not validate the return values from
flb_sds_cat_safe, but identical concatenations inmeminfo_configure(lines 100, 108, 134) check for errors. If either concatenation fails,tmpbecomes invalid, causing undefined behavior when passed toflb_hash_table_get. Add error checks consistent with the rest of the file:Suggested fix
else if (parts == 3) { /* Compose new metric name */ tmp = flb_sds_create_size(256); - flb_sds_cat_safe(&tmp, metric_name, flb_sds_len(metric_name) - 1); - flb_sds_cat_safe(&tmp, "_bytes", 6); + ret = flb_sds_cat_safe(&tmp, metric_name, flb_sds_len(metric_name) - 1); + if (ret != 0) { + flb_sds_destroy(tmp); + flb_slist_destroy(&split_list); + continue; + } + ret = flb_sds_cat_safe(&tmp, "_bytes", 6); + if (ret != 0) { + flb_sds_destroy(tmp); + flb_slist_destroy(&split_list); + continue; + } /* Get metric context */🤖 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/in_node_exporter_metrics/ne_meminfo_linux.c` around lines 260 - 263, In meminfo_update, the two flb_sds_cat_safe calls that append to tmp (the flb_sds created with flb_sds_create_size) need their return values checked like in meminfo_configure; if flb_sds_cat_safe returns FLB_SDS_ERROR (or NULL/invalid per project convention) free tmp and return an error (or propagate failure) instead of continuing to call flb_hash_table_get with an invalid tmp. Update the flb_sds_cat_safe invocations in meminfo_update to validate the result, clean up tmp on failure, and return early to match the error handling used in meminfo_configure.
🤖 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/in_elasticsearch/in_elasticsearch_bulk_prot.c`:
- Around line 173-180: Before appending a comma to bulk_statuses in the loop
(the call to bulk_statuses_cat when idx > 0 && idx % 2 == 0), ensure there is
enough space for both the comma and the minimum following status payload by
calling status_buffer_avail(ctx, *bulk_statuses, 51) (or equivalent threshold)
first; only call bulk_statuses_cat(...) if that pre-check returns FLB_TRUE,
otherwise break to avoid leaving a trailing comma. This change touches the
comma-insertion branch (the idx/%2 check and bulk_statuses_cat call) and the
subsequent status_buffer_avail check so order and threshold are updated to
prevent producing ",]" in constrained-buffer cases.
In `@plugins/out_bigquery/bigquery.c`:
- Around line 140-148: The code calls bigquery_jwt_base64_url_encode(..., &olen)
and immediately uses olen/buf without checking the helper's return value; modify
both call sites (the block using variables buf, buf_size, payload, len, olen,
tmp, out) to capture the helper's return (e.g., rc =
bigquery_jwt_base64_url_encode(..., &olen)) and if rc == -1 fail fast by jumping
to the existing error cleanup path (goto error) instead of using a stale olen or
appending buf; ensure the same change is applied to the other occurrence around
lines 185-197 so both paths consistently validate the encoder result before
calling flb_sds_cat.
In `@plugins/out_stackdriver/stackdriver.c`:
- Around line 293-303: The code calls jwt_base64_url_encode((unsigned char *)
buf, buf_size, (unsigned char *) payload, len, &olen) but ignores its return
value and proceeds to use olen when appending via flb_sds_cat, which can use a
stale olen on failure; update the blocks around jwt_base64_url_encode (both
occurrences) to check the function's return (e.g., rc or ret) for failure, jump
to the existing error cleanup label if encoding failed, and only call
flb_sds_cat(out, buf, olen) and assign tmp = ... when the encode call succeeded;
reference the jwt_base64_url_encode, olen, buf, buf_size, payload, flb_sds_cat,
out, tmp, and the error label to locate and implement the change.
---
Outside diff comments:
In `@plugins/in_node_exporter_metrics/ne_meminfo_linux.c`:
- Around line 260-263: In meminfo_update, the two flb_sds_cat_safe calls that
append to tmp (the flb_sds created with flb_sds_create_size) need their return
values checked like in meminfo_configure; if flb_sds_cat_safe returns
FLB_SDS_ERROR (or NULL/invalid per project convention) free tmp and return an
error (or propagate failure) instead of continuing to call flb_hash_table_get
with an invalid tmp. Update the flb_sds_cat_safe invocations in meminfo_update
to validate the result, clean up tmp on failure, and return early to match the
error handling used in meminfo_configure.
🪄 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: 8025c4f6-e3e2-4572-99ab-3dbec5fe115e
📒 Files selected for processing (11)
plugins/in_elasticsearch/in_elasticsearch_bulk_prot.cplugins/in_node_exporter_metrics/ne_meminfo_linux.cplugins/out_azure/azure.cplugins/out_azure/azure_conf.cplugins/out_azure_kusto/azure_kusto.cplugins/out_azure_kusto/azure_msiauth.cplugins/out_bigquery/bigquery.cplugins/out_stackdriver/gce_metadata.cplugins/out_stackdriver/stackdriver.cplugins/out_stackdriver/stackdriver_conf.ctests/integration/scenarios/in_elasticsearch/tests/test_in_elasticsearch_001.py
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
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
1 participant
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.
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
Bug Fixes
Tests