Use full time_t size (which is 64bit on 64bit architectures) for time#11545
Use full time_t size (which is 64bit on 64bit architectures) for time#11545dirkmueller wants to merge 1 commit intofluent:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMacros and local variables for log-event timestamps were changed from 32-bit types to Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_log_event_decoder.c (1)
458-470:⚠️ Potential issue | 🟠 MajorReorder sentinel checks before the non-negative branch to ensure portability.
Line 462's
if (s >= 0)check assumestime_tis signed. On platforms with unsignedtime_t, the cast(time_t)-1and(time_t)-2produce large positive numbers, making the condition always true. This prevents the GROUP_START and GROUP_END branches from ever executing, silently misclassifying group markers as normal records.Reorder the comparisons to check the sentinel values first, then fall back to the
>= 0case for normal records. This works correctly regardless oftime_tsignedness.Suggested fix
- if (s >= 0) { - *type = FLB_LOG_EVENT_NORMAL; - return 0; - } - else if (s == FLB_LOG_EVENT_GROUP_START) { + if (s == FLB_LOG_EVENT_GROUP_START) { *type = FLB_LOG_EVENT_GROUP_START; return 0; } else if (s == FLB_LOG_EVENT_GROUP_END) { *type = FLB_LOG_EVENT_GROUP_END; return 0; } + else if (s >= 0) { + *type = FLB_LOG_EVENT_NORMAL; + return 0; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_log_event_decoder.c` around lines 458 - 470, Reorder the sentinel checks so the special-group markers are tested before the non-negative-time branch: inspect the value assigned to s (s = event->timestamp.tm.tv_sec) and first compare it against FLB_LOG_EVENT_GROUP_START and FLB_LOG_EVENT_GROUP_END, setting *type to FLB_LOG_EVENT_GROUP_START or FLB_LOG_EVENT_GROUP_END respectively and returning 0; only after those sentinel comparisons fail, check if s >= 0 and set *type = FLB_LOG_EVENT_NORMAL. This ensures correct behavior even if time_t is unsigned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/flb_log_event_decoder.c`:
- Around line 370-371: The debug log uses "%ld" to print
event->timestamp.tm.tv_sec (a time_t) which is not portable; update the
flb_debug call that contains event->timestamp.tm.tv_sec to use a portable
integer format (e.g. "%jd") and cast the value to (intmax_t) so the format
matches across platforms (locate the flb_debug invocation in
flb_log_event_decoder.c that references event->timestamp.tm.tv_sec and replace
the format specifier and add the cast).
---
Outside diff comments:
In `@src/flb_log_event_decoder.c`:
- Around line 458-470: Reorder the sentinel checks so the special-group markers
are tested before the non-negative-time branch: inspect the value assigned to s
(s = event->timestamp.tm.tv_sec) and first compare it against
FLB_LOG_EVENT_GROUP_START and FLB_LOG_EVENT_GROUP_END, setting *type to
FLB_LOG_EVENT_GROUP_START or FLB_LOG_EVENT_GROUP_END respectively and returning
0; only after those sentinel comparisons fail, check if s >= 0 and set *type =
FLB_LOG_EVENT_NORMAL. This ensures correct behavior even if time_t is unsigned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a74d8209-f907-4e12-bd1f-f50cf18a34d3
📒 Files selected for processing (2)
include/fluent-bit/flb_log_event.hsrc/flb_log_event_decoder.c
dirkmueller
force-pushed
the
fix_int32_in_time
branch
3 times, most recently
from
9d4f6a5 to
c0f6316
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/flb_log_event_decoder.c (1)
316-317:⚠️ Potential issue | 🟡 MinorUse a portable format specifier for
invalid_timestamp.
invalid_timestampisint64_t, but%ldexpectslong. That is still a varargs mismatch on LLP64 targets and can misprint this debug path. Please verify this on Windows/LLP64 as well;%jdwith an(intmax_t)cast is portable.Suggested fix
- flb_debug("[decoder] Invalid group marker timestamp (%ld), skipping record. " - "Group state preserved.", invalid_timestamp); + flb_debug("[decoder] Invalid group marker timestamp (%jd), skipping record. " + "Group state preserved.", (intmax_t) invalid_timestamp);Also applies to: 371-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_log_event_decoder.c` around lines 316 - 317, The debug print uses the int64_t variable invalid_timestamp with a non-portable "%ld" specifier; change the printf/format calls that print invalid_timestamp (and the other occurrences noted around the later debug prints) to use "%jd" and cast the value to (intmax_t) (e.g., printf("... %jd ...", (intmax_t)invalid_timestamp)) to ensure correct printing across LLP64/LP64 platforms; update every formatted occurrence that references invalid_timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/flb_log_event_decoder.c`:
- Around line 460-475: The ext timestamp decode is being truncated by the
int32_t cast in flb_log_event_decoder_decode_timestamp, so update the ext decode
path to decode the timestamp into a wider integer (e.g., int64_t or time_t)
instead of casting to int32_t; remove the blanket (int32_t) cast and preserve
the full 64-bit value (or sign-extend correctly) when assigning to
event->timestamp.tm.tv_sec, and in that same ext decode codepath explicitly
detect the reserved 32-bit group-marker encodings (the values corresponding to
FLB_LOG_EVENT_GROUP_START and FLB_LOG_EVENT_GROUP_END) before any sign/narrowing
conversion so those markers are handled correctly by flb_log_event_decoder_next
and not misclassified due to post-2038 negative values.
---
Duplicate comments:
In `@src/flb_log_event_decoder.c`:
- Around line 316-317: The debug print uses the int64_t variable
invalid_timestamp with a non-portable "%ld" specifier; change the printf/format
calls that print invalid_timestamp (and the other occurrences noted around the
later debug prints) to use "%jd" and cast the value to (intmax_t) (e.g.,
printf("... %jd ...", (intmax_t)invalid_timestamp)) to ensure correct printing
across LLP64/LP64 platforms; update every formatted occurrence that references
invalid_timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64bb35be-9295-46a7-882e-b889bd3dc0cf
📒 Files selected for processing (2)
include/fluent-bit/flb_log_event.hsrc/flb_log_event_decoder.c
🚧 Files skipped from review as they are similar to previous changes (1)
- include/fluent-bit/flb_log_event.h
dirkmueller
force-pushed
the
fix_int32_in_time
branch
from
c0f6316 to
c642e80
Compare
(Fixes# fluent#11532) Signed-off-by: Dirk Müller <dirk@dmllr.de>
dirkmueller
force-pushed
the
fix_int32_in_time
branch
from
c642e80 to
fa5d2c7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/flb_log_event_decoder.c (2)
197-205:⚠️ Potential issue | 🔴 CriticalPreserve the 32-bit group-marker encodings before widening
tv_sec.The ext payload is still a 32-bit wire value, and
src/flb_log_event_encoder.c:190-215writesFLB_LOG_EVENT_GROUP_START/FLB_LOG_EVENT_GROUP_ENDthrough auint32_tcast. Casting that raw value straight totime_tzero-extends0xffffffff/0xfffffffeon 64-bit builds, so valid group markers become large positive timestamps andflb_log_event_decoder_get_record_type()treats them as normal records.Suggested fix
- output->tm.tv_sec = - (time_t) FLB_UINT32_TO_HOST_BYTE_ORDER( - FLB_ALIGNED_DWORD_READ( - (unsigned char *) &input->via.ext.ptr[0])); + { + uint32_t raw_sec; + + raw_sec = FLB_UINT32_TO_HOST_BYTE_ORDER( + FLB_ALIGNED_DWORD_READ( + (unsigned char *) &input->via.ext.ptr[0])); + + if (raw_sec == (uint32_t) FLB_LOG_EVENT_GROUP_START) { + output->tm.tv_sec = FLB_LOG_EVENT_GROUP_START; + } + else if (raw_sec == (uint32_t) FLB_LOG_EVENT_GROUP_END) { + output->tm.tv_sec = FLB_LOG_EVENT_GROUP_END; + } + else { + output->tm.tv_sec = (time_t) raw_sec; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_log_event_decoder.c` around lines 197 - 205, The decoder currently casts the 32-bit wire values directly to time_t which widens values like FLB_LOG_EVENT_GROUP_START/END into large 64-bit timestamps; fix by first reading the two 32-bit fields into uint32_t temporaries using FLB_UINT32_TO_HOST_BYTE_ORDER(FLB_ALIGNED_DWORD_READ(...)) (e.g. uint32_t sec32 and nsec32), then assign output->tm.tv_sec = (time_t) sec32 and output->tm.tv_nsec = (time_t) nsec32 so the original 32-bit encodings (and comparisons in flb_log_event_decoder_get_record_type against FLB_LOG_EVENT_GROUP_START/END) are preserved before widening.
371-373:⚠️ Potential issue | 🟡 MinorUse a portable format specifier for
invalid_timestamp.
invalid_timestampisint64_t, but%ldonly matcheslong. That reintroduces the same portability bug on LLP64 targets; use%jd/PRId64with the matching cast instead.Suggested fix
+#include <inttypes.h> ... - flb_debug("[decoder] Invalid group marker timestamp (%ld), skipping record. " - "Group state preserved.", invalid_timestamp); + flb_debug("[decoder] Invalid group marker timestamp (%jd), skipping record. " + "Group state preserved.", + (intmax_t) invalid_timestamp);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_log_event_decoder.c` around lines 371 - 373, The flb_debug call prints invalid_timestamp (declared as int64_t) using %ld which is non-portable; change the format to use a proper int64 specifier (e.g., PRId64 or %jd) and cast the value accordingly (cast to intmax_t for %jd or use the PRId64 macro with the int64_t value) in the flb_debug invocation that logs invalid_timestamp computed from event->timestamp.tm.tv_sec so the format and argument type match across platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/flb_log_event_decoder.c`:
- Around line 197-205: The decoder currently casts the 32-bit wire values
directly to time_t which widens values like FLB_LOG_EVENT_GROUP_START/END into
large 64-bit timestamps; fix by first reading the two 32-bit fields into
uint32_t temporaries using
FLB_UINT32_TO_HOST_BYTE_ORDER(FLB_ALIGNED_DWORD_READ(...)) (e.g. uint32_t sec32
and nsec32), then assign output->tm.tv_sec = (time_t) sec32 and
output->tm.tv_nsec = (time_t) nsec32 so the original 32-bit encodings (and
comparisons in flb_log_event_decoder_get_record_type against
FLB_LOG_EVENT_GROUP_START/END) are preserved before widening.
- Around line 371-373: The flb_debug call prints invalid_timestamp (declared as
int64_t) using %ld which is non-portable; change the format to use a proper
int64 specifier (e.g., PRId64 or %jd) and cast the value accordingly (cast to
intmax_t for %jd or use the PRId64 macro with the int64_t value) in the
flb_debug invocation that logs invalid_timestamp computed from
event->timestamp.tm.tv_sec so the format and argument type match across
platforms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9202a981-edd8-4124-89ca-a7d7f2390aca
📒 Files selected for processing (2)
include/fluent-bit/flb_log_event.hsrc/flb_log_event_decoder.c
🚧 Files skipped from review as they are similar to previous changes (1)
- include/fluent-bit/flb_log_event.h
There was a problem hiding this comment.
This change cannot be merged as-is. Widening tv_sec to time_t without redefining the wire encoding breaks the coherence between serialization and deserialization.
In particular, sentinel values reserved for group markers now overlap with valid second values in the timestamp domain, and the decoder reinterprets valid packed second values as control markers.
We need a coherent encoding contract first, rather than only widening the in-memory type.
2 participants
Fixes #11532
Summary by CodeRabbit