Repository navigation
time-util: fix operator precedence in format_timestamp_style() size check - #44676
Merged
Merged
Conversation
…heck
The minimum buffer size check in format_timestamp_style() lacked
parentheses around the conditional expression. Since + binds more
tightly than == and ?:, it was parsed as
(3 + 1 + 10 + style == TIMESTAMP_DATE) ? 0 : (1 + 8 + ... + 1)
i.e. the weekday and date were never accounted for, and the check only
required 12 to 21 bytes instead of 15 to 35. Usually the strftime()
calls catch a buffer that is too short later on. However, for timestamps
that are too large to be formatted, the "--- XXXX-XX-XX ..." placeholder
is copied with only an assert() guarding the buffer size, so e.g. a 20
byte buffer with TIMESTAMP_PRETTY resulted in an assertion failure
instead of NULL being returned (or in a buffer overflow with NDEBUG).
All callers in the tree use FORMAT_TIMESTAMP_MAX sized buffers, hence
this is not triggered in practice, but let's fix the check and add a
test.
|
Reviewed bd48cc1 (architecture, correctness, style; failed: lifetimes, security, tests). Suggestions
Nits
The review is incomplete, so the next push reviews the full diff again. |
github-actions
Bot
added
tests
util-lib
please-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
poettering
added
good-to-merge/with-minor-suggestions
and removed
please-review
Member
|
Please address the review comments by Claude. |
yuwata
approved these changes
Nov 10, 2026
b84330808
force-pushed
the
fix/time-util-timestamp-size-check
branch
from
November 10, 2026 08:01
844988f to
bd48cc1
Compare
Contributor
Author
|
rebased to main Validation: test-time-util passes. With only the test changes applied, the new assertion fails for the PRETTY placeholder, confirming the regression test catches the original issue. |
yuwata
added
good-to-merge/waiting-for-ci 👍
github-actions
Bot
removed
the
good-to-merge/waiting-for-ci 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The minimum buffer size check in format_timestamp_style() was missing parentheses around the ?: expression, so it was parsed as (3 + 1 + 10 + style == TIMESTAMP_DATE) ? 0 : ..., which never counted the weekday and the date. The check required 12 to 21 bytes instead of 15 to 35.
For normal timestamps the later strftime() calls still catch a short buffer, but for timestamps too large to format the "--- XXXX-XX-XX ..." placeholder is copied with only an assert() guarding the size, so a buffer of 12 to 23 bytes hits the assertion instead of getting NULL back (and would overflow the buffer with NDEBUG). All in-tree callers pass FORMAT_TIMESTAMP_MAX sized buffers, so this is not triggered in practice. I also grepped src/ for the same a + b == C ? x : y pattern; this is the only instance.
The new test calls format_timestamp_style() with every buffer size from 0 up to the result length for each style, checks it returns NULL and does not write past the given size, and checks the full-size result. After rebasing onto current main, meson test test-time-util passes, as do test-calendarspec and test-date.
Found by reading the code; there is no upstream issue for this. Prepared with AI assistance, as in #44652.