in_kmsg: fix /dev/kmsg parsing #10807
Conversation
WalkthroughReworks kmsg input parsing to use unsigned conversions and stricter consumption/delimiter checks for priority, sequence, and timestamp; uses 64-bit-safe arithmetic for timestamp math; adds immediate error handling with fail path and buffer rollback; removes unused Changes
Sequence Diagram(s)sequenceDiagram
participant Kernel as Kernel log line
participant Parser as in_kmsg.c Parser
participant Buffer as Buffer/State
Kernel->>Parser: Provide raw kmsg line
Parser->>Parser: strtoul(priority) -> val
alt parse error / no digits / ERANGE
Parser-->>Buffer: goto fail (ctx->buffer_id--)
Parser->>Kernel: discard line
else valid priority
Parser->>Parser: derive FLB_KLOG_PRI(val)
Parser->>Parser: advance, strtoull(sequence)
alt seq error / missing comma
Parser-->>Buffer: goto fail (ctx->buffer_id--)
Parser->>Kernel: discard line
else valid sequence
Parser->>Parser: advance, strtoull(timestamp)
alt timestamp error / missing comma
Parser-->>Buffer: goto fail (ctx->buffer_id--)
Parser->>Kernel: discard line
else valid timestamp
Parser->>Parser: tv_sec = val / KMSG_USEC_PER_SEC
Parser->>Parser: tv_usec = val % KMSG_USEC_PER_SEC
Parser->>Buffer: commit parsed record
end
end
end
note right of Parser: Added strict delimiter & overflow checks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
plugins/in_kmsg/in_kmsg.c (1)
179-180: tv_usec calc fix is correct; consider simpler modulo form and constant.Functionally correct and 64-bit safe. For readability/consistency with KMSG_USEC_PER_SEC, modulo is clearer.
Apply:
- tv.tv_sec = val/1000000; - tv.tv_usec = val - ((uint64_t)tv.tv_sec * 1000000); + tv.tv_sec = val / KMSG_USEC_PER_SEC; + tv.tv_usec = val % KMSG_USEC_PER_SEC;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugins/in_kmsg/in_kmsg.c(3 hunks)
🔇 Additional comments (3)
plugins/in_kmsg/in_kmsg.c (3)
125-130: Good switch to unsigned parsing with correct overflow guard.Using strtoul for priority with the ULONG_MAX/ERANGE check is correct for an unsigned field.
137-137: Priority derivation from unsigned value looks right.Mapping via FLB_KLOG_PRI(val) after unsigned parsing aligns with the field semantics.
157-162: Nice: strict sequence parsing with delimiter check.The end != p and immediate ',' enforcement eliminates ambiguous inputs.
Also applies to: 164-165
plugins/in_kmsg/in_kmsg.c
Outdated
| /* ensure something was consumed */ | ||
| if (end == p) { | ||
| goto fail; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also enforce semicolon adjacency after timestamp (drop the later strchr).
Mirror the sequence check for the timestamp so we don’t accept stray characters between the number and ';'. Advance p via end and remove the subsequent scan.
Apply:
/* ensure something was consumed */
if (end == p) {
goto fail;
}
+ /* after the timestamp, the next char must be ';' */
+ if (*end != ';') {
+ goto fail;
+ }
+ p = ++end;
+
- /* Now process the human readable message */
- p = strchr(p, ';');
- if (!p) {
- goto fail;
- }
- p++;
+ /* Now process the human readable message */Also applies to: 183-189
🤖 Prompt for AI Agents
In plugins/in_kmsg/in_kmsg.c around lines 173-177 (and similarly for 183-189),
the code only checks that the timestamp parse advanced p but then later searches
for ';' with strchr allowing stray chars; instead set p = end immediately after
parsing the number and then require that *p == ';' (advance p++ after checking)
so no intervening characters are permitted; remove the subsequent strchr() call
and its logic and mirror this exact sequence for the other timestamp parse at
183-189.
* use strtoul for priority since it is unsigned * check for overflow correctly by comparing against ULONG_MAX * fail if no digits were consumed or next character is not comma * calculate tv.tv_usec correctly in case tv.tv_sec 32-bit * remove redundant errno = 0 added in 3254b9a * remove erroneous time.h added with 72d9dc8 Signed-off-by: Erik Karlsson <[email protected]>
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:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
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