Conversation
📝 WalkthroughWalkthroughAdds a public atomic API with platform backends, container containment/ownership to prevent cycles, pervasive NULL/overflow hardening, CI and documentation updates, CMake/test wiring, and extensive unit tests covering concurrency and edge cases. ChangesCFL Core Library Enhancement
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
|
![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: c80159a0c0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "GNU") | ||
| set(PLATFORM_SPECIFIC_ATOMIC_MODULE cfl_atomic_gcc.c) |
There was a problem hiding this comment.
When CFL is built with GCC/Clang on targets that cannot inline 64-bit __atomic_* operations (for example several 32-bit embedded architectures), this branch selects cfl_atomic_gcc.c/cfl_atomic_clang.c but never adds -latomic; the new implementation uses 64-bit __atomic_compare_exchange, __atomic_store_n, and __atomic_load_n, so executables that reference the new cfl_atomic_* API can fail at link time with unresolved __atomic_*_8 symbols. Please either probe/link atomic for these compiler branches or fall back to the mutex implementation when the builtins are not linkable.
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
🧹 Nitpick comments (1)
lib/cfl/AGENTS.md (1)
1-77: ⚡ Quick winKeep vendor bump PRs free of local process docs
Please avoid introducing
lib/cfl/AGENTS.mdin this dependency-upgrade PR. For Line 1 onward, this adds repo-local workflow policy inside vendored code and will likely increase drift/merge friction on future upstream CFL syncs. Consider moving this guidance to a top-level maintainer doc (or a separate PR) and keeping this PR limited to thev0.7.0vendor update.Based on learnings: "Prefer minimal patches that avoid unrelated formatting or refactoring churn" and "Do not mix unrelated code and documentation updates in one commit unless explicitly requested."
🤖 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 `@lib/cfl/AGENTS.md` around lines 1 - 77, Remove lib/cfl/AGENTS.md from this vendor bump PR and keep the change focused on the v0.7.0 dependency update; either relocate the guidance into a top-level maintainer document (e.g., MAINTAINERS.md) or open a separate PR for workflow/process docs, and ensure the commit/PR only contains the vendor upgrade files referenced in this diff.
🤖 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 `@lib/cfl/src/cfl_sds.c`:
- Around line 227-229: The bounds check in cfl_sds.c for self-append slice
validation is off-by-one: replace the conditional that currently uses "if
(append_len - 1 > head->alloc - source_offset)" with a comparison using ">=" so
that when the source slice ends exactly at head->alloc it is rejected; update
the check around the variables append_len, source_offset and head->alloc in the
same block (the self-append slice validation) to use ">=" to prevent reading
s[head->alloc].
---
Nitpick comments:
In `@lib/cfl/AGENTS.md`:
- Around line 1-77: Remove lib/cfl/AGENTS.md from this vendor bump PR and keep
the change focused on the v0.7.0 dependency update; either relocate the guidance
into a top-level maintainer document (e.g., MAINTAINERS.md) or open a separate
PR for workflow/process docs, and ensure the commit/PR only contains the vendor
upgrade files referenced in this diff.
🪄 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: f9000ea8-4e50-4221-a359-e81fc01b0ac0
📒 Files selected for processing (45)
lib/cfl/.github/workflows/build.yamllib/cfl/.github/workflows/lint.yamllib/cfl/.github/workflows/packages.yamllib/cfl/AGENTS.mdlib/cfl/CMakeLists.txtlib/cfl/README.mdlib/cfl/include/cfl/cfl.hlib/cfl/include/cfl/cfl_array.hlib/cfl/include/cfl/cfl_atomic.hlib/cfl/include/cfl/cfl_checksum.hlib/cfl/include/cfl/cfl_container.hlib/cfl/include/cfl/cfl_kv.hlib/cfl/include/cfl/cfl_kvlist.hlib/cfl/include/cfl/cfl_list.hlib/cfl/include/cfl/cfl_object.hlib/cfl/include/cfl/cfl_sds.hlib/cfl/include/cfl/cfl_time.hlib/cfl/include/cfl/cfl_utils.hlib/cfl/include/cfl/cfl_variant.hlib/cfl/src/CMakeLists.txtlib/cfl/src/cfl.clib/cfl/src/cfl_array.clib/cfl/src/cfl_atomic_clang.clib/cfl/src/cfl_atomic_gcc.clib/cfl/src/cfl_atomic_generic.clib/cfl/src/cfl_atomic_msvc.clib/cfl/src/cfl_checksum.clib/cfl/src/cfl_container.clib/cfl/src/cfl_kv.clib/cfl/src/cfl_kvlist.clib/cfl/src/cfl_object.clib/cfl/src/cfl_sds.clib/cfl/src/cfl_utils.clib/cfl/src/cfl_variant.clib/cfl/tests/CMakeLists.txtlib/cfl/tests/array.clib/cfl/tests/atomic_operations.clib/cfl/tests/checksum.clib/cfl/tests/headers.clib/cfl/tests/kv.clib/cfl/tests/kvlist.clib/cfl/tests/object.clib/cfl/tests/sds.clib/cfl/tests/utils.clib/cfl/tests/variant.c
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@lib/cfl/CMakeLists.txt`:
- Around line 6-10: The CMake version variables currently set
(CFL_VERSION_MAJOR, CFL_VERSION_MINOR, CFL_VERSION_PATCH) compute
CFL_VERSION_STR as "0.7.1" which mismatches the PR text; either set
CFL_VERSION_PATCH to 0 so CFL_VERSION_STR becomes "0.7.0" or update the PR
title/description to "v0.7.1" for consistency—locate and edit the entries
CFL_VERSION_PATCH and CFL_VERSION_STR in lib/cfl/CMakeLists.txt (or update PR
metadata) so package naming and release artifacts match the declared version.
In `@lib/cfl/src/cfl_container.c`:
- Around line 475-519: The function cfl_container_move_variant_to_array must
reject variants already owned by another container: check variant->owned (and
return -1) early—before calling claim_variant_container() and before modifying
any parent pointers; do the same change in the sibling helper (the
move-to-kvlist function, e.g. cfl_container_move_variant_to_kvlist) so both
helpers fail fast on variant->owned to prevent double-ownership/double-free/UAF.
In `@lib/cfl/src/CMakeLists.txt`:
- Around line 65-71: The Windows atomic fallback currently forces
PLATFORM_SPECIFIC_ATOMIC_MODULE to cfl_atomic_msvc.c on CFL_SYSTEM_WINDOWS which
uses MSVC-only intrinsics (e.g., _InterlockedCompareExchange64) and fails for
MinGW/Clang/GCC; update the CMake logic to only select cfl_atomic_msvc.c when
the compiler is MSVC (e.g., CMAKE_C_COMPILER_ID MATCHES "MSVC") and otherwise
fall back to cfl_atomic_generic.c or set CFL_ATOMIC_NEEDS_THREADS On so non‑MSVC
Windows builds use the compiler-agnostic implementation; modify the branch
around CFL_SYSTEM_WINDOWS/CFL_ATOMIC_BUILTINS_LINK_WITH_LIBATOMIC to gate
PLATFORM_SPECIFIC_ATOMIC_MODULE selection by the compiler ID.
In `@lib/cfl/tests/kvlist.c`:
- Around line 1254-1267: The test must return immediately on setup failures
instead of proceeding because TEST_CHECK doesn't abort; after creating the list
(cfl_kvlist_create), after inserting the string (cfl_kvlist_insert_string),
after creating the tmp file (tmpfile) and after printing (cfl_kvlist_print)
check the return values and if any indicate failure, free any allocated
resources (e.g., destroy the list) and return from the test function so
subsequent calls (like compare which uses fp) cannot dereference NULL or invalid
handles; specifically add early returns when list == NULL, ret != 0 for
cfl_kvlist_insert_string, fp == NULL after tmpfile, and ret <= 0 for
cfl_kvlist_print, cleaning up as needed before returning.
🪄 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: e2aef14a-3075-4d16-8e7a-64b6e00395d9
📒 Files selected for processing (45)
lib/cfl/.github/workflows/build.yamllib/cfl/.github/workflows/lint.yamllib/cfl/.github/workflows/packages.yamllib/cfl/AGENTS.mdlib/cfl/CMakeLists.txtlib/cfl/README.mdlib/cfl/include/cfl/cfl.hlib/cfl/include/cfl/cfl_array.hlib/cfl/include/cfl/cfl_atomic.hlib/cfl/include/cfl/cfl_checksum.hlib/cfl/include/cfl/cfl_container.hlib/cfl/include/cfl/cfl_kv.hlib/cfl/include/cfl/cfl_kvlist.hlib/cfl/include/cfl/cfl_list.hlib/cfl/include/cfl/cfl_object.hlib/cfl/include/cfl/cfl_sds.hlib/cfl/include/cfl/cfl_time.hlib/cfl/include/cfl/cfl_utils.hlib/cfl/include/cfl/cfl_variant.hlib/cfl/src/CMakeLists.txtlib/cfl/src/cfl.clib/cfl/src/cfl_array.clib/cfl/src/cfl_atomic_clang.clib/cfl/src/cfl_atomic_gcc.clib/cfl/src/cfl_atomic_generic.clib/cfl/src/cfl_atomic_msvc.clib/cfl/src/cfl_checksum.clib/cfl/src/cfl_container.clib/cfl/src/cfl_kv.clib/cfl/src/cfl_kvlist.clib/cfl/src/cfl_object.clib/cfl/src/cfl_sds.clib/cfl/src/cfl_utils.clib/cfl/src/cfl_variant.clib/cfl/tests/CMakeLists.txtlib/cfl/tests/array.clib/cfl/tests/atomic_operations.clib/cfl/tests/checksum.clib/cfl/tests/headers.clib/cfl/tests/kv.clib/cfl/tests/kvlist.clib/cfl/tests/object.clib/cfl/tests/sds.clib/cfl/tests/utils.clib/cfl/tests/variant.c
✅ Files skipped from review due to trivial changes (10)
- lib/cfl/.github/workflows/lint.yaml
- lib/cfl/include/cfl/cfl_utils.h
- lib/cfl/include/cfl/cfl_object.h
- lib/cfl/include/cfl/cfl_time.h
- lib/cfl/include/cfl/cfl_kv.h
- lib/cfl/include/cfl/cfl_checksum.h
- lib/cfl/include/cfl/cfl_atomic.h
- lib/cfl/AGENTS.md
- lib/cfl/README.md
- lib/cfl/include/cfl/cfl.h
🚧 Files skipped from review as they are similar to previous changes (20)
- lib/cfl/tests/headers.c
- lib/cfl/src/cfl_atomic_gcc.c
- lib/cfl/src/cfl_checksum.c
- lib/cfl/tests/kv.c
- lib/cfl/tests/checksum.c
- lib/cfl/include/cfl/cfl_sds.h
- lib/cfl/src/cfl.c
- lib/cfl/src/cfl_atomic_clang.c
- lib/cfl/include/cfl/cfl_list.h
- lib/cfl/tests/array.c
- lib/cfl/.github/workflows/packages.yaml
- lib/cfl/tests/object.c
- lib/cfl/src/cfl_atomic_msvc.c
- lib/cfl/src/cfl_atomic_generic.c
- lib/cfl/src/cfl_sds.c
- lib/cfl/src/cfl_kv.c
- lib/cfl/tests/CMakeLists.txt
- lib/cfl/.github/workflows/build.yaml
- lib/cfl/tests/atomic_operations.c
- lib/cfl/src/cfl_kvlist.c
| # C Floppy Version | ||
| set(CFL_VERSION_MAJOR 0) | ||
| set(CFL_VERSION_MINOR 6) | ||
| set(CFL_VERSION_MINOR 7) | ||
| set(CFL_VERSION_PATCH 1) | ||
| set(CFL_VERSION_STR "${CFL_VERSION_MAJOR}.${CFL_VERSION_MINOR}.${CFL_VERSION_PATCH}") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of the cfl library (C Floppy library)?
💡 Result:
The “C Floppy library” (cfl) appears to refer to the fluent/cfl C-flavored library. Its latest released version is v0.6.1 (marked “Latest” and dated May 22, 2025 on the repository page). [1] Note: there are other unrelated software projects abbreviated “cfl” (e.g., “Causal Feature Learning”, “Compressed File Library”, etc.), so the name is ambiguous; the fluent/cfl repository is the only one in the sources explicitly describing “c:\ floppy” as “CFL”. [2]
Citations:
Update PR title and description or adjust version numbers for consistency.
The version configured in the code (0.7.1) does not match the PR title and description, which state "upgrade to v0.7.0." The computed version string will be "0.7.1", which flows through to package naming and release artifacts. Either update CFL_VERSION_PATCH to 0 (to match the stated v0.7.0), or update the PR title/description to "v0.7.1" for consistency.
Note: The upstream cfl library's latest released version is v0.6.1; the 0.7.x version does not yet have an official release.
🤖 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 `@lib/cfl/CMakeLists.txt` around lines 6 - 10, The CMake version variables
currently set (CFL_VERSION_MAJOR, CFL_VERSION_MINOR, CFL_VERSION_PATCH) compute
CFL_VERSION_STR as "0.7.1" which mismatches the PR text; either set
CFL_VERSION_PATCH to 0 so CFL_VERSION_STR becomes "0.7.0" or update the PR
title/description to "v0.7.1" for consistency—locate and edit the entries
CFL_VERSION_PATCH and CFL_VERSION_STR in lib/cfl/CMakeLists.txt (or update PR
metadata) so package naming and release artifacts match the declared version.
| int cfl_container_move_variant_to_array(struct cfl_array *array, | ||
| struct cfl_variant *variant) | ||
| { | ||
| struct cfl_array *child_array; | ||
| struct cfl_kvlist *child_kvlist; | ||
|
|
||
| if (array == NULL || variant == NULL) { | ||
| return -1; | ||
| } | ||
|
|
||
| if (variant->type == CFL_VARIANT_ARRAY) { | ||
| child_array = variant->data.as_array; | ||
|
|
||
| if (child_array != NULL && | ||
| parent_chain_contains_array(array, NULL, child_array)) { | ||
| return -1; | ||
| } | ||
| } | ||
| else if (variant->type == CFL_VARIANT_KVLIST) { | ||
| child_kvlist = variant->data.as_kvlist; | ||
|
|
||
| if (child_kvlist != NULL && | ||
| parent_chain_contains_kvlist(array, NULL, child_kvlist)) { | ||
| return -1; | ||
| } | ||
| } | ||
|
|
||
| if (claim_variant_container(variant) != 0) { | ||
| return -1; | ||
| } | ||
|
|
||
| if (variant->type == CFL_VARIANT_ARRAY && | ||
| variant->data.as_array != NULL) { | ||
| variant->data.as_array->parent_array = array; | ||
| variant->data.as_array->parent_kvlist = NULL; | ||
| } | ||
| else if (variant->type == CFL_VARIANT_KVLIST && | ||
| variant->data.as_kvlist != NULL) { | ||
| variant->data.as_kvlist->parent_array = array; | ||
| variant->data.as_kvlist->parent_kvlist = NULL; | ||
| } | ||
|
|
||
| variant->owned = CFL_TRUE; | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
Reject already-owned variants before reparenting.
Both move helpers can accept a variant that is already stored in another container. That lets two parents own and later destroy the same struct cfl_variant *, which is a double-free/UAF path. Fail fast on variant->owned before claiming or updating parent pointers.
Suggested fix
int cfl_container_move_variant_to_array(struct cfl_array *array,
struct cfl_variant *variant)
{
struct cfl_array *child_array;
struct cfl_kvlist *child_kvlist;
if (array == NULL || variant == NULL) {
return -1;
}
+ if (variant->owned) {
+ return -1;
+ }
if (variant->type == CFL_VARIANT_ARRAY) {
child_array = variant->data.as_array;
@@
int cfl_container_move_variant_to_kvlist(struct cfl_kvlist *kvlist,
struct cfl_variant *variant)
{
struct cfl_array *child_array;
struct cfl_kvlist *child_kvlist;
if (kvlist == NULL || variant == NULL) {
return -1;
}
+ if (variant->owned) {
+ return -1;
+ }
if (variant->type == CFL_VARIANT_ARRAY) {
child_array = variant->data.as_array;Also applies to: 522-566
🤖 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 `@lib/cfl/src/cfl_container.c` around lines 475 - 519, The function
cfl_container_move_variant_to_array must reject variants already owned by
another container: check variant->owned (and return -1) early—before calling
claim_variant_container() and before modifying any parent pointers; do the same
change in the sibling helper (the move-to-kvlist function, e.g.
cfl_container_move_variant_to_kvlist) so both helpers fail fast on
variant->owned to prevent double-ownership/double-free/UAF.
| if(CFL_ATOMIC_BUILTINS_LINK_WITH_LIBATOMIC) | ||
| set(CFL_ATOMIC_NEEDS_LIBATOMIC On) | ||
| elseif(CFL_SYSTEM_WINDOWS) | ||
| set(PLATFORM_SPECIFIC_ATOMIC_MODULE cfl_atomic_msvc.c) | ||
| else() | ||
| set(PLATFORM_SPECIFIC_ATOMIC_MODULE cfl_atomic_generic.c) | ||
| set(CFL_ATOMIC_NEEDS_THREADS On) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== Atomic fallback branch =="
sed -n '51,74p' lib/cfl/src/CMakeLists.txt
echo
echo "== cfl_atomic_msvc.c (inspect for MSVC-only intrinsics/guards) =="
sed -n '1,200p' lib/cfl/src/cfl_atomic_msvc.c
echo
echo "== Other atomic backends present =="
fd '^cfl_atomic_.*\.c$' lib/cfl/srcRepository: fluent/fluent-bit
Length of output: 5147
Fix Windows atomic fallback to support non-MSVC compilers.
The 64-bit path in cfl_atomic_msvc.c uses MSVC intrinsics (_InterlockedCompareExchange64, _InterlockedExchange64, _InterlockedOr64), which are unavailable on MinGW and other non-MSVC Windows toolchains. If the builtin probe fails on Windows with GCC/Clang, this fallback will fail to compile.
Either:
- Gate the
cfl_atomic_msvc.cbackend on a compiler check (e.g.,CMAKE_C_COMPILER_ID MATCHES "MSVC"), or - Reimplement the 64-bit atomic operations using compiler-agnostic Windows APIs (critical section or synchronization APIs) like the 32-bit path does.
🤖 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 `@lib/cfl/src/CMakeLists.txt` around lines 65 - 71, The Windows atomic fallback
currently forces PLATFORM_SPECIFIC_ATOMIC_MODULE to cfl_atomic_msvc.c on
CFL_SYSTEM_WINDOWS which uses MSVC-only intrinsics (e.g.,
_InterlockedCompareExchange64) and fails for MinGW/Clang/GCC; update the CMake
logic to only select cfl_atomic_msvc.c when the compiler is MSVC (e.g.,
CMAKE_C_COMPILER_ID MATCHES "MSVC") and otherwise fall back to
cfl_atomic_generic.c or set CFL_ATOMIC_NEEDS_THREADS On so non‑MSVC Windows
builds use the compiler-agnostic implementation; modify the branch around
CFL_SYSTEM_WINDOWS/CFL_ATOMIC_BUILTINS_LINK_WITH_LIBATOMIC to gate
PLATFORM_SPECIFIC_ATOMIC_MODULE selection by the compiler ID.
| list = cfl_kvlist_create(); | ||
| TEST_CHECK(list != NULL); | ||
|
|
||
| ret = cfl_kvlist_insert_string(list, "a\"b\n", "v\n"); | ||
| TEST_CHECK(ret == 0); | ||
|
|
||
| fp = tmpfile(); | ||
| TEST_CHECK(fp != NULL); | ||
|
|
||
| ret = cfl_kvlist_print(fp, list); | ||
| TEST_CHECK(ret > 0); | ||
|
|
||
| ret = compare(fp, "{\"a\\\"b\\n\":\"v\\n\"}"); | ||
| TEST_CHECK(ret == 0); |
There was a problem hiding this comment.
Return immediately after setup failures in this test.
TEST_CHECK does not abort. If tmpfile() fails here, compare(fp, ...) dereferences fp through fseek() and the test can crash instead of reporting a normal failure.
Suggested fix
list = cfl_kvlist_create();
- TEST_CHECK(list != NULL);
+ if (!TEST_CHECK(list != NULL)) {
+ return;
+ }
ret = cfl_kvlist_insert_string(list, "a\"b\n", "v\n");
TEST_CHECK(ret == 0);
fp = tmpfile();
- TEST_CHECK(fp != NULL);
+ if (!TEST_CHECK(fp != NULL)) {
+ cfl_kvlist_destroy(list);
+ return;
+ }🤖 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 `@lib/cfl/tests/kvlist.c` around lines 1254 - 1267, The test must return
immediately on setup failures instead of proceeding because TEST_CHECK doesn't
abort; after creating the list (cfl_kvlist_create), after inserting the string
(cfl_kvlist_insert_string), after creating the tmp file (tmpfile) and after
printing (cfl_kvlist_print) check the return values and if any indicate failure,
free any allocated resources (e.g., destroy the list) and return from the test
function so subsequent calls (like compare which uses fp) cannot dereference
NULL or invalid handles; specifically add early returns when list == NULL, ret
!= 0 for cfl_kvlist_insert_string, fp == NULL after tmpfile, and ret <= 0 for
cfl_kvlist_print, cleaning up as needed before returning.
1 participant
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
New Features
Bug Fixes
Documentation
Version
Tests
Chores