[Bug libstdc++/111163] New: signed integer overflow in std::format("{:%S}",std::chrono::duration....)

[Bug libstdc++/111163] New: signed integer overflow in std::format("{:%S}",std::chrono::duration....)

[gcc at pauldreik dot se]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111163

            Bug ID: 111163
           Summary: signed integer overflow in
                    std::format("{:%S}",std::chrono::duration....)
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gcc at pauldreik dot se
  Target Milestone: ---

The following program:

#include <chrono>
#include <format>
int main() {
    [[maybe_unused]] auto blah = std::format(
        "{:%S}", std::chrono::duration<long, std::kilo>{2314885530818453536});
}

invokes signed integer overflow when compiled with the current gcc trunk. The
output is:
/opt/compiler-explorer/gcc-trunk-20230824/include/c++/14.0.0/bits/chrono.h:190:8:
runtime error: signed integer overflow: 2314885530818453536 * 5 cannot be
represented in type 'long int'
/opt/compiler-explorer/gcc-trunk-20230824/include/c++/14.0.0/bits/chrono.h:229:38:
runtime error: signed integer overflow: 2314885530818453536 * 5 cannot be
represented in type 'long int'

link to reproducer: https://godbolt.org/z/YWd9cx7o8

[Bug libstdc++/111163] signed integer overflow in std::format("{:%S}",std::chrono::duration....)

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111163

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.3
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
   Last reconfirmed|                            |2023-08-26

[Bug libstdc++/111163] signed integer overflow in std::format("{:%S}",std::chrono::duration....)

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111163

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This doesn't seem much different to:
  std::chrono::seconds s = std::chrono::duration<long,
std::kilo>{2314885530818453536};
Which we can't really do much about.

[Bug libstdc++/111163] signed integer overflow in std::format("{:%S}",std::chrono::duration....)

[gcc at pauldreik dot se]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111163

--- Comment #2 from Paul Dreik <gcc at pauldreik dot se> ---
The fmt lib had the same problem. I wrote a safe duration cast which eventually
morphed into this:

https://github.com/fmtlib/fmt/blob/9b74160817f2bc63288d2111e823a35dd3dbf234/include/fmt/chrono.h#L57-L68

[Bug libstdc++/111163] signed integer overflow in std::format("{:%S}",std::chrono::duration....)

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111163

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
We could also just have a "can_hh_mm_ss" check, as I think we use hh_mm_ss in
most places where we do these conversations. Then we would just check once, not
on every cast. I don't think we want to change the conversions inside the
hh_mm_ss constructor

[Bug libstdc++/111163] signed integer overflow in std::format("{:%S}",std::chrono::duration....)

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111163

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
With this patch:

--- a/libstdc++-v3/include/bits/chrono_io.h
+++ b/libstdc++-v3/include/bits/chrono_io.h
@@ -507,6 +507,16 @@ namespace __format
          if constexpr (__is_specialization_of<_Tp, chrono::hh_mm_ss>)
            __is_neg = __t.is_negative();

+         if constexpr (chrono::__is_duration_v<_Tp>)
+           if constexpr (_Tp::period::num != 1)
+             {
+               // Check conversion to seconds will not overflow.
+               chrono::seconds::rep __s{};
+               if (__builtin_mul_overflow(__t.count(), _Tp::period::num,
&__s))
+                 __throw_format_error("chrono format error: integer overflow "
+                                      "converting duration to seconds");
+             }
+
          auto __print_sign = [&__is_neg, &__out] {
            if constexpr (chrono::__is_duration_v<_Tp>
                            || __is_specialization_of<_Tp, chrono::hh_mm_ss>)

The testcase throws:

terminate called after throwing an instance of 'std::format_error'
  what():  chrono format error: integer overflow converting duration to seconds
Aborted (core dumped)


This isn't ideal as there are some uses that wouldn't overflow, e.g.
std::format("{:%j}", std::chrono::days::max())
This doesn't require any conversion and so could be formatted. But I'm not sure
I care about this case, it's pretty unlikely to ever be needed.

If we did the overflow checks at the point of conversion then we'd be doing
them multiple times for a format string like "%H:%M:%S". But then I suppose
we're already converting the duration to hh_mm_ss three times there anyway. The
patch in PR 110739 comment 2 would help with that, as the _S_hms call might be
subject to common subexpression elimination, and the checks would only be done
once. We could also just call _S_hms once up-front when we have a type that can
make use of it, and then reuse the result as needed.


And the patch above doesn't handle all cases anyway, because this will still
overflow:
std::format("{:%S}", std::chrono::sys_days{std::chrono::days::max()});

So maybe we just need a checked duration_cast<days>, floor<days>, floor<weeks>,
and the check for conversion to seconds for use with hh_mm_ss.

[Bug libstdc++/111163] signed integer overflow in std::format("{:%S}",std::chrono::duration....)

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111163

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.3                        |13.4

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 13.3 is being released, retargeting bugs to GCC 13.4.