-
Notifications
You must be signed in to change notification settings - Fork 1.8k
pack_json: fix data type assignation #10913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Eduardo Silva <[email protected]>
WalkthroughAdds variables to capture return value and state-derived output size when using the JSMN backend with an existing state, conditionally updating the caller-provided out_size only on success, and returning the captured status. Resets state_out_size before incremental read handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Pack as flb_pack_json.c
participant State as flb_pack_json_state
Caller->>Pack: flb_pack_json(json, len, out_buf, out_size, state)
alt Backend = JSMN and state exists
Pack->>State: invoke(json, len, out_buf, state_out_size)
State-->>Pack: ret, state_out_size
alt ret == 0
Note over Pack: Update *out_size = state_out_size
else ret != 0
Note over Pack: Do not modify out_size
end
Pack-->>Caller: return ret
else Other paths
Pack-->>Caller: existing behavior
end
rect rgba(230, 246, 255, 0.5)
Note over Pack: Reset state_out_size = 0 before incremental read handling
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/flb_pack_json.c (1)
30-33
: Narrow variable scope and drop redundant zeroing.ret and state_out_size are only used in the JSMN+state branch. Limit their scope and remove the extra assignment.
- int ret; int backend; - int state_out_size = 0; struct flb_pack_state *state = NULL; @@ - if (state) { - state_out_size = 0; + if (state) { + int ret; + int state_out_size = 0;Also applies to: 50-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_pack_json.c
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/flb_pack_json.c (1)
src/flb_pack.c (1)
flb_pack_json_state
(613-684)
⏰ 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_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- 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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=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_ARROW=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_SIMD=On, 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_UNDEFINED=On, 3.31.6, clang, clang++)
- 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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 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 - fuzzing test
- 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-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
🔇 Additional comments (1)
src/flb_pack_json.c (1)
57-64
: Good fix: remove size_t/int pointer mismatch and gate out_size updates on success.Capturing into a local int and assigning to size_t only on success avoids UB on 64‑bit and prevents stale values from leaking through. rg shows many callers still pass &out_size — verify those callers declare an int (not size_t). Run this to locate callers that may be passing a size_t address:
#!/bin/bash set -euo pipefail rg -nP '\bflb_pack_json_state\s*\(' -n --hidden | while IFS=: read -r file lineno _; do start=$((lineno-40)); [ $start -lt 1 ] && start=1 echo "== $file:$lineno ==" sed -n "${start},$((lineno+3))p" "$file" sed -n "${start},$((lineno))p" "$file" | rg -nP --color=never 'size_t\s+\w+' || true done
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