Conversation
📝 WalkthroughWalkthroughAdded buffer boundary validations in the MQTT publish message handler to prevent unsafe memory reads. The function now checks available bytes before reading topic length, packet identifier (when QoS > 0), and payload data, returning early on insufficient data with appropriate error logging. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: ce8dc0a885
ℹ️ 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".
| if (conn->buf_pos > conn->buf_frame_end) { | ||
| flb_plg_debug(ctx->ins, "truncated publish payload"); | ||
| return -1; |
There was a problem hiding this comment.
buf_pos can legitimately advance to buf_frame_end + 1 when a PUBLISH frame contains only the topic (and optional packet id) with an empty payload, so this condition turns a valid no-payload publish into -1. In the current flow that propagates through mqtt_prot_parser() and mqtt_conn_event() into a connection teardown (plugins/in_mqtt/mqtt_prot.c:453-456, plugins/in_mqtt/mqtt_conn.c:63-66). Before this change the same case simply reached mqtt_data_append() with msg_len == 0 and was dropped/logged, so clients that use empty publishes (for example retained-message clears/tombstones) now regress from a no-op to a disconnect.
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/in_mqtt/mqtt_prot.c`:
- Around line 329-332: The truncation check in mqtt_prot.c incorrectly rejects a
valid zero-length PUBLISH payload because it treats conn->buf_pos ==
conn->buf_frame_end + 1 as truncated; update the condition to allow that
boundary by only treating positions beyond that as truncation (change the check
that currently uses conn->buf_pos and conn->buf_frame_end so it tests
conn->buf_pos > conn->buf_frame_end + 1), keeping the existing
flb_plg_debug("truncated publish payload") path for true truncation cases.
- Around line 278-281: The checks in mqtt_prot.c that use BUF_AVAIL() and
conn->buf_len (e.g., the branch that logs "truncated publish topic length" and
the hlen comparison) must be changed to use frame-local bounds so a truncated
PUBLISH cannot borrow bytes from the next coalesced frame; calculate available
bytes as (conn->buf_frame_end - conn->buf_pos) or otherwise use
conn->buf_frame_end when validating lengths, replace uses of BUF_AVAIL() and
conn->buf_len in the relevant validation blocks (the truncated-topic-length
check and the hlen vs buf_len check) with that frame-local available value, and
return -1 on failure as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7835c61a-039e-4a84-99f7-c1814a7e0f2f
📒 Files selected for processing (1)
plugins/in_mqtt/mqtt_prot.c
| if (BUF_AVAIL() < 2) { | ||
| flb_plg_debug(ctx->ins, "truncated publish topic length"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Use frame-local bounds here, not BUF_AVAIL() / conn->buf_len.
Line 278 and Line 301 validate against the full receive buffer, and Line 288 compares hlen against conn->buf_len. With coalesced packets, a truncated current PUBLISH can still consume bytes from the next frame before failing. These checks should be bounded by conn->buf_frame_end.
Proposed patch
- if (BUF_AVAIL() < 2) {
+ if (conn->buf_pos + 1 > conn->buf_frame_end) {
flb_plg_debug(ctx->ins, "truncated publish topic length");
return -1;
}
@@
- if (hlen > (conn->buf_len - conn->buf_pos)) {
+ if (hlen > (conn->buf_frame_end - conn->buf_pos)) {
flb_plg_debug(ctx->ins, "invalid topic length");
return -1;
}
@@
- if (BUF_AVAIL() < 2) {
+ if (conn->buf_pos + 1 > conn->buf_frame_end) {
flb_plg_debug(ctx->ins, "truncated publish packet id");
return -1;
}Also applies to: 288-291, 301-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/in_mqtt/mqtt_prot.c` around lines 278 - 281, The checks in
mqtt_prot.c that use BUF_AVAIL() and conn->buf_len (e.g., the branch that logs
"truncated publish topic length" and the hlen comparison) must be changed to use
frame-local bounds so a truncated PUBLISH cannot borrow bytes from the next
coalesced frame; calculate available bytes as (conn->buf_frame_end -
conn->buf_pos) or otherwise use conn->buf_frame_end when validating lengths,
replace uses of BUF_AVAIL() and conn->buf_len in the relevant validation blocks
(the truncated-topic-length check and the hlen vs buf_len check) with that
frame-local available value, and return -1 on failure as before.
| if (conn->buf_pos > conn->buf_frame_end) { | ||
| flb_plg_debug(ctx->ins, "truncated publish payload"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Current payload truncation check rejects valid empty PUBLISH payloads.
Line 329 treats conn->buf_pos == conn->buf_frame_end + 1 as truncation, but that is a valid zero-byte payload boundary in MQTT. This can disconnect clients for legal packets.
Proposed patch
- if (conn->buf_pos > conn->buf_frame_end) {
+ if (conn->buf_pos > conn->buf_frame_end + 1) {
flb_plg_debug(ctx->ins, "truncated publish payload");
return -1;
}📝 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.
| if (conn->buf_pos > conn->buf_frame_end) { | |
| flb_plg_debug(ctx->ins, "truncated publish payload"); | |
| return -1; | |
| } | |
| if (conn->buf_pos > conn->buf_frame_end + 1) { | |
| flb_plg_debug(ctx->ins, "truncated publish payload"); | |
| return -1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/in_mqtt/mqtt_prot.c` around lines 329 - 332, The truncation check in
mqtt_prot.c incorrectly rejects a valid zero-length PUBLISH payload because it
treats conn->buf_pos == conn->buf_frame_end + 1 as truncated; update the
condition to allow that boundary by only treating positions beyond that as
truncation (change the check that currently uses conn->buf_pos and
conn->buf_frame_end so it tests conn->buf_pos > conn->buf_frame_end + 1),
keeping the existing flb_plg_debug("truncated publish payload") path for true
truncation cases.
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
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
Bug Fixes