in_storage_backlog: fix use-after-free when segregating chunk#10892
Merged
in_storage_backlog: fix use-after-free when segregating chunk#10892
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
WalkthroughThe change updates cb_queue_chunks in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Q as cb_queue_chunks
participant SB as Segregated Backlog
participant CIO as cio_chunk (underlying_chunk)
rect rgba(230,240,255,0.5)
note over Q: Handling chunk (corrupted/empty/map-fail or final queue)
Q->>Q: derive underlying_chunk = chunk_instance->chunk
Q->>SB: remove(underlying_chunk)
alt corrupted/empty/map-read failure
Q->>CIO: close(underlying_chunk)
else final queueing/downgrade
Q->>CIO: down(underlying_chunk)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (2)
plugins/in_storage_backlog/sb.c (2)
564-572: Fixes UAF order; confirm deletion policy for corrupted chunks.
Order is now correct (unlink from backlogs before close). But closing with FLB_FALSE leaves a corrupted, now-unreferenced chunk on disk/stream (not re-segregated), potentially leaking disk space. Consider honoring storage_del_bad_chunks (and error type) here too.Apply:
- sb_remove_chunk_from_segregated_backlogs(underlying_chunk, ctx); - cio_chunk_close(underlying_chunk, FLB_FALSE); + sb_remove_chunk_from_segregated_backlogs(underlying_chunk, ctx); + { + int chunk_error = cio_error_get(underlying_chunk); + int del = (ctx->ins->config->storage_del_bad_chunks == FLB_TRUE) && + (chunk_error == CIO_ERR_BAD_FILE_SIZE || + chunk_error == CIO_ERR_BAD_LAYOUT); + cio_chunk_close(underlying_chunk, del ? FLB_TRUE : FLB_FALSE); + }
625-628: Mapping-failure path fixed; minor consistency nit.
Sequence is safe. For consistency with other paths, set underlying_chunk before the down/close and use it for cio_chunk_down too.- cio_chunk_down(chunk_instance->chunk); + underlying_chunk = chunk_instance->chunk; + cio_chunk_down(underlying_chunk); ... - underlying_chunk = chunk_instance->chunk; - sb_remove_chunk_from_segregated_backlogs(underlying_chunk, ctx); - cio_chunk_close(underlying_chunk, FLB_TRUE); + sb_remove_chunk_from_segregated_backlogs(underlying_chunk, ctx); + cio_chunk_close(underlying_chunk, FLB_TRUE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_storage_backlog/sb.c(5 hunks)
⏰ 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). (29)
- 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 32bit, x86, x86-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_SANITIZE_MEMORY=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_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 (-DFLB_SIMD=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_ARROW=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_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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=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_JEMALLOC=Off, 3.31.6, clang, clang++)
- 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=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- 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, clang, clang++, ubuntu-24.04, clang-14)
- 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-compile-centos-7
- GitHub Check: PR - fuzzing test
🔇 Additional comments (3)
plugins/in_storage_backlog/sb.c (3)
521-521: Good defensive pointer: prevents UAF.
Using a local underlying_chunk decouples the CIO chunk lifetime from the sb_out_chunk wrapper. LGTM.
593-596: Correct: unlink-before-close removes UAF on metadata errors.
Safe sequencing and use of underlying_chunk look good.
606-609: Correct: unlink-before-close for empty chunks.
This eliminates the UAF reported in #10868 for the empty-path as well. LGTM.
runderwo
reviewed
Sep 18, 2025
| sb_remove_chunk_from_segregated_backlogs(chunk_instance->chunk, ctx); | ||
| /* This function will indirecly release chunk_instance so it has to be | ||
| * called last. | ||
| underlying_chunk = chunk_instance->chunk; |
Contributor
There was a problem hiding this comment.
This indent is wrong/misleading, isn't it?
7 tasks
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
2 participants
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.
Fixes #10868
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