Skip to content

Fix overflow in tm formatter - #2564

Merged
vitaut merged 1 commit into
fmtlib:masterfrom
phprus:optimize-tm-formatting-3-ub-2
Oct 27, 2021
Merged

Fix overflow in tm formatter#2564
vitaut merged 1 commit into
fmtlib:masterfrom
phprus:optimize-tm-formatting-3-ub-2

Conversation

@phprus

@phprus phprus commented Oct 24, 2021

Copy link
Copy Markdown
Contributor

Fix possible overflow errors in tm formatter on invalid tm values.

@phprus phprus mentioned this pull request Oct 24, 2021
Merged
@phprus
phprus force-pushed the optimize-tm-formatting-3-ub-2 branch 3 times, most recently from 0a1a08e to 0465eca Compare October 25, 2021 15:10
@phprus

phprus commented Oct 26, 2021

Copy link
Copy Markdown
Contributor Author

@vitaut, review this PR please.
This is fixing year overflow and possible overflow on all digits2 calls (is tm is invalid).

vitaut
vitaut reviewed Oct 27, 2021
View reviewed changes

@vitaut vitaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. Looks good overall, just a few minor comments inline.

Comment thread include/fmt/chrono.h Outdated

auto tm_year() const noexcept -> int { return 1900 + tm_.tm_year; }
auto tm_sec() const noexcept -> int {
FMT_ASSERT(tm_.tm_sec >= 0 && tm_.tm_sec <= 60, "");

@vitaut vitaut Oct 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://pubs.opengroup.org/onlinepubs/7908799/xsh/time.h.html tm_sec should be in the range [0, 61] so the upper bound should be 61 here.

@toughengineer toughengineer Oct 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's [0..60] since C99 and C++11.
https://en.cppreference.com/w/c/chrono/tm
https://en.cppreference.com/w/cpp/chrono/c/tm

@phprus phprus Oct 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For compatibility, I changed the interval to [0, 61].

Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h
return tm_.tm_yday;
}

auto tm_hour12() const noexcept -> int {

@vitaut vitaut Oct 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest merging this function into on_12_hour since it's not used anywhere else.

@phprus phprus Oct 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function tm_hour12 will be used in on_12_hour_time. PR #2550 (https://github.com/fmtlib/fmt/pull/2550/files#diff-5d08eff445a9a7793fbd22d873b2284f0ae69f1550aff06f673de49768bccca0R1744-R1750)

@vitaut vitaut Oct 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's keep it then.

Comment thread include/fmt/chrono.h
@phprus
Add FMT_ASSERT and validation of values of struct tm members
75f7dd2 
Switch internal year calculations to long long
@phprus
phprus force-pushed the optimize-tm-formatting-3-ub-2 branch from 0465eca to 75f7dd2 Compare October 27, 2021 20:14
@phprus

phprus commented Oct 27, 2021

Copy link
Copy Markdown
Contributor Author

@vitaut
Something broke on documentation build...
https://github.com/fmtlib/fmt/runs/4027135638?check_suite_focus=true

Exception occurred:
  File "/home/runner/work/fmt/build/virtualenv/lib/python3.8/site-packages/docutils/writers/html5_polyglot/__init__.py", line 445, in section_title_tags
    if (ids and self.settings.section_self_link
AttributeError: 'Values' object has no attribute 'section_self_link'

@vitaut
vitaut merged commit 90034e4 into fmtlib:master Oct 27, 2021
@vitaut

vitaut commented Oct 27, 2021

Copy link
Copy Markdown
Contributor

The documentation issue looks unrelated, merged.

PoetaKodu pushed a commit to pacc-repo/fmt that referenced this pull request Nov 11, 2021
@phprus @PoetaKodu
Add FMT_ASSERT and validation of values of struct tm members (fmtlib#…
314e4e6 
…2564)

Switch internal year calculations to long long
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vitaut vitaut vitaut left review comments
+1 more reviewer
@toughengineer toughengineer toughengineer left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@phprus @vitaut @toughengineer