Skip to content

Modify vsnprintf condition checks for size#11878

Open
vast0906 wants to merge 1 commit into
fluent:masterfrom
vast0906:master
Open

Modify vsnprintf condition checks for size#11878
vast0906 wants to merge 1 commit into
fluent:masterfrom
vast0906:master

Conversation

@vast0906
Copy link
Copy Markdown

@vast0906 vast0906 commented May 28, 2026

Fixes #11876


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:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

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
    • Improved string formatting and buffer handling to fix off-by-one and boundary edge cases, ensuring proper error reporting and preventing rare truncation or crash scenarios for very long formatted strings, enhancing stability and reliability.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b6d52361-f52f-4604-9ba3-f41828d70235

📥 Commits

Reviewing files that changed from the base of the PR and between d774f5e and b9b3704.

📒 Files selected for processing (1)
  • src/flb_sds.c
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/flb_sds.c

📝 Walkthrough

Walkthrough

Two small fixes in SDS formatting: flb_sds_printf treats exact-fit as insatiable; flb_sds_snprintf returns -1 for negative vsnprintf and adjusts buffer-growth math to grow when ret >= size by ret - size + 1 and set size = ret + 1.

Changes

SDS Buffer Boundary Handling

Layer / File(s) Summary
SDS format function boundary fixes
src/flb_sds.c
flb_sds_printf changes the insatiable vsnprintf check from > to >= at available-space boundaries. flb_sds_snprintf returns -1 when vsnprintf yields ret < 0, grows the SDS when (size_t)ret >= size by ret - size + 1 (accounting for NUL), and sets size = ret + 1.

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested labels:
backport to v4.2.x

Suggested reviewers:

  • edsiper
  • cosmo0920
  • braydonk

I hopped through buffers late at night,
Counting bytes by soft moonlight,
Fixed the gap where NUL once slipped,
Now strings land whole — not half-equipped,
A quiet hop, a tidy byte! 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: modifying vsnprintf condition checks for handling buffer size edge cases in flb_sds functions.
Linked Issues check ✅ Passed Changes to vsnprintf boundary conditions in flb_sds.c directly address the root cause of issue #11876: off-by-one errors in buffer handling causing NUL/truncation in ES bulk output formatting.
Out of Scope Changes check ✅ Passed All changes are confined to src/flb_sds.c with focused modifications to buffer handling logic; no unrelated refactoring or feature additions detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/flb_sds.c`:
- Around line 405-414: The vsnprintf retry path in flb_sds_snprintf can
mis-handle negative returns and leaks the va_list on error; after calling
vsnprintf (in the retry_snprintf block) first check if ret < 0 and if so call
va_end(va) and return -1, then perform the resize check using a size-safe
comparison (e.g. if ((size_t)ret >= size) or cast ret to size_t) before
computing the grow amount; also ensure any early returns (like tmp == NULL) call
va_end(va) before returning; update references to flb_sds_increase and the
va_list handling in flb_sds_snprintf accordingly.
🪄 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: ff636f22-8d35-4a1b-a723-ebcd18438202

📥 Commits

Reviewing files that changed from the base of the PR and between 19cc8f9 and 972ff20.

📒 Files selected for processing (1)
  • src/flb_sds.c

Comment thread src/flb_sds.c
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

Actionable comments posted: 0

@cosmo0920
Copy link
Copy Markdown
Contributor

We need to some prefixes for these changes in your commit:


Run python .github/scripts/commit_prefix_check.py

❌ Commit d774f5e4db failed:
Missing prefix in commit subject: 'Fix error handling in retry_snprintf function'

❌ Commit 972ff206d5 failed:
Missing prefix in commit subject: 'Modify vsnprintf condition checks for size'


Commit prefix validation failed.

@cosmo0920
Copy link
Copy Markdown
Contributor

Plus, we encourage to add unit test cases for most of the fixing/adding functionality cases.
In this fix, we should add the following test cases or the equivalents.

Here is a suggested direction for adding the test cases:

1. Test for flb_sds_snprintf

The bug in flb_sds_snprintf happened when the length of the string to write exactly matched the size argument passed to vsnprintf.

You can suggest adding a test like this to tests/internal/sds.c:

static void test_sds_snprintf_truncation()
{
    flb_sds_t s;
    int ret;
    /* Exactly 5 characters long */
    char *expected = "12345";

    /* Create an SDS string with a capacity of 5 bytes */
    s = flb_sds_create_size(5);

    /* 
     * flb_sds_alloc(s) is 5.
     * vsnprintf will attempt to write "12345" + NUL.
     * It writes "1234" + NUL and returns 5.
     * With the old buggy code, ret (5) == size (5), so it skipped reallocation.
     */
    ret = flb_sds_snprintf(&s, flb_sds_alloc(s), "%s", expected);

    /* Assert that it correctly returned 5 and reallocated space */
    TEST_CHECK(ret == 5);
    TEST_CHECK(flb_sds_len(s) == 5);
    
    /* With the old code, `s` would physically contain "1234\0" and this would fail */
    TEST_CHECK(strcmp(s, expected) == 0);

    flb_sds_destroy(s);
}

2. Test for flb_sds_printf

For flb_sds_printf, there is an aggressive upfront reallocation if the available space is less than 64 (if (len < 64) len = 64;).
To test the exact edge case where size == flb_sds_avail(s), the length of the final string we are printing must be exactly 64 characters (matching the upfront minimum size).

static void test_sds_printf_truncation()
{
    flb_sds_t s;
    flb_sds_t tmp;
    int len;
    
    /* String of exactly 64 characters */
    char *expected = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_+";

    /* Capacity 64 */
    s = flb_sds_create_size(64);
    
    /* 
     * flb_sds_avail(s) is 64. 
     * vsnprintf will write 63 chars + NUL, and return 64.
     * With the old buggy code, size (64) == flb_sds_avail (64), so it skipped reallocation.
     */
    tmp = flb_sds_printf(&s, "%s", expected);

    TEST_CHECK(tmp != NULL);
    len = flb_sds_len(s);
    TEST_CHECK(len == 64);
    
    /* With the old code, `s` would end with '_' instead of '+', and this would fail */
    TEST_CHECK(strcmp(s, expected) == 0);

    flb_sds_destroy(s);
}

3. Registering the Tests

Finally, these functions should be appended to the TEST_LIST array at the bottom of tests/internal/sds.c:

TEST_LIST = {
    // ... existing tests
    { "sds_snprintf_truncation", test_sds_snprintf_truncation},
    { "sds_printf_truncation", test_sds_printf_truncation},
    { 0 }
};

These two cases isolate the specific boundary conditions where the string being formatted is precisely the length of the currently allocated space, validating that the new >= operators catch the truncation and successfully enforce a buffer resize.

Updated conditions in vsnprintf checks to include equality.

Signed-off-by: vast0906 <vast0906@gmail.com>

Fix error handling in retry_snprintf function

Signed-off-by: vast0906 <vast0906@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[out_es] Id_Key + Write_Operation produces unparseable bulk body ("no requests added") on 4.2.4

2 participants