out_es, out_opensearch: fix compose_index_header stack overflow - #12127
out_es, out_opensearch: fix compose_index_header stack overflow#12127saddamr3e wants to merge 1 commit into
Conversation
Signed-off-by: Saddam <saddamr3e@gmail.com>
📝 WalkthroughWalkthroughThe Elasticsearch and OpenSearch outputs now calculate Logstash index append offsets from the copied index string. Runtime tests cover truncation of overlong ChangesLogstash index formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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: 78205fec2f
ℹ️ 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".
| p = logstash_index + es_index_custom_len; | ||
| } else { | ||
| p = logstash_index + flb_sds_len(ctx->logstash_prefix); | ||
| p = logstash_index + strlen(logstash_index); |
There was a problem hiding this comment.
When logstash_prefix_separator is configured so the first compose_index_header() call fails, elasticsearch_format() retries with "-" on the same logstash_index buffer. This new strlen(logstash_index) then starts the retry after the partially written failed separator/date instead of after the static prefix, so the fallback can leave an index like prefix<oversized-separator> with no date rather than rebuilding prefix-YYYY...; the OpenSearch copy has the same issue. Please keep the truncated prefix length before mutating the buffer, or reset the buffer before retrying.
Useful? React with 👍 / 👎.
![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_es/es.c`:
- Around line 267-269: Replace the dynamic strlen-based offset with the bounded
flb_sds_len(ctx->logstash_prefix) calculation in compose_index_header for
plugins/out_es/es.c lines 267-269 and plugins/out_opensearch/opensearch.c lines
257-259, preserving the prefix-based append position across retries.
🪄 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: 75336672-43da-4945-a2d7-941637a193c4
📒 Files selected for processing (4)
plugins/out_es/es.cplugins/out_opensearch/opensearch.ctests/runtime/out_elasticsearch.ctests/runtime/out_opensearch.c
| } else { | ||
| p = logstash_index + flb_sds_len(ctx->logstash_prefix); | ||
| p = logstash_index + strlen(logstash_index); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Avoid strlen to prevent state corruption and undefined behavior on retry.
Both Elasticsearch and OpenSearch retry compose_index_header with a default separator if the first attempt fails. Using strlen(logstash_index) calculates the append offset dynamically based on the buffer's current contents. If the first attempt fails (e.g., during strftime), snprintf has already written the custom separator to the buffer. On retry, strlen will include this failed write, causing the new separator to append rather than overwrite. Furthermore, if strftime fails, the array contents are undefined per POSIX and may not be null-terminated, potentially causing strlen to read out of bounds.
To safely fix the overflow without introducing this retry bug, calculate the bounded prefix length directly from ctx->logstash_prefix.
plugins/out_es/es.c#L267-L269: Replace thestrlencalculation with a boundedflb_sds_len(ctx->logstash_prefix).plugins/out_opensearch/opensearch.c#L257-L259: Replace thestrlencalculation with a boundedflb_sds_len(ctx->logstash_prefix).
🐛 Proposed fix for both files
- p = logstash_index + strlen(logstash_index);
+ size_t default_len = flb_sds_len(ctx->logstash_prefix);
+ if (default_len > logstash_index_size - 1) {
+ default_len = logstash_index_size - 1;
+ }
+ p = logstash_index + default_len;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| p = logstash_index + flb_sds_len(ctx->logstash_prefix); | |
| p = logstash_index + strlen(logstash_index); | |
| } | |
| } else { | |
| size_t default_len = flb_sds_len(ctx->logstash_prefix); | |
| if (default_len > logstash_index_size - 1) { | |
| default_len = logstash_index_size - 1; | |
| } | |
| p = logstash_index + default_len; | |
| } |
📍 Affects 2 files
plugins/out_es/es.c#L267-L269(this comment)plugins/out_opensearch/opensearch.c#L257-L259
🤖 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_es/es.c` around lines 267 - 269, Replace the dynamic strlen-based
offset with the bounded flb_sds_len(ctx->logstash_prefix) calculation in
compose_index_header for plugins/out_es/es.c lines 267-269 and
plugins/out_opensearch/opensearch.c lines 257-259, preserving the prefix-based
append position across retries.
1 participant
compose_index_header builds the Logstash index inside a 256-byte on-stack buffer (logstash_index). When logstash_prefix_key is unset it takes the write offset from flb_sds_len(ctx->logstash_prefix), the full prefix length, but the prefix was already strncpy-truncated into that 256-byte buffer a few lines earlier. A logstash_prefix of 256 bytes or more pushes the offset past the buffer, logstash_index_size - len underflows to a large size_t, and the snprintf and strftime that follow write out of bounds. The per-record logstash_prefix_key path already caps its copy at 128 bytes, so only the config-prefix branch is affected. out_opensearch carries the same code and the same bug. Taking the offset from strlen(logstash_index), the length actually present in the buffer, keeps the write in bounds and leaves the result unchanged for prefixes that fit.
Testing
Added
tests/runtime/out_elasticsearch.c:logstash_format_long_prefixandtests/runtime/out_opensearch.c:logstash_format_long_prefix, each configuringlogstash_formatwith a 300-byte prefix. Built with-DSANITIZE_ADDRESS=On, both abort on the unpatched tree and pass after the change (the index becomes the safely truncated prefix). ASan on the unpatched tree:ok-package-testlabel to test for all targets.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
Bug Fixes
Tests