[m-rev.] for review: overflow checks for string conversions in the calendar module

[Julien Fischer]

On Sat, 15 Aug 2026 at 02:43, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
>
>
>
> On Sat, 15 Aug 2026 01:36:07 +1000, Julien Fischer <jfischer at opturion.com> wrote:
>
> > +    % This predicate fails if
> > +    % - the string does not conform to the above format;
> > +    % - any date or time component is outside its valid range;
> > +    % - the absolute value of the year cannot be represented by Mercury's
> > +    %   int type.
> >      %
> >  :- pred date_time_from_string(string::in, date_time::out) is semidet.
>
> Add "or" before the third item.

Done.

>
> > @@ -460,8 +463,11 @@
> >      % This fraction component cannot include more than six digits, since
> >      % the maximum resolution of a duration is a microsecond.
> >      %
> > -    % Fail if the string does not conform to the above format, or if the
> > -    % fractional part of the seconds component has more than six digits.
> > +    % This predicates fails if
> > +    % - the string does not conform to the above format;
> > +    % - the fractional part of the seconds component has more than six digits;
> > +    % - any of the duration components cannot be represented by Mercury's
> > +    %   int type.
> >      %
> >      % For example, the duration 1 year, 18 months, 100 days, 10 hours, 15
> >      % minutes, 90 seconds and 300 microseconds can be written as:
>
> Same here.

Done.

> > +    % See the comment for string.do_base_string_to_positive_int_loop/8
> > +    % for an explanation of how we check for overflow here.
> > +    CutOff = max_int `unchecked_quotient` 10,
> > +    CutLimit = max_int `unchecked_rem` 10,
> > +    read_int_and_return_num_digits_loop(CutOff, CutLimit, 0, Int,
> > +        0, NumDigits, !Chars).
>
> I would use the naming scheme I proposed in my previous review.

There didn't seem to be a consensus on what to call them.
When there is, I will rename them all.

> I see that the diff tests for the third failure condition of both
> modified exported predicates, but where are the tests for the
> second conditions, for component valid range and six digits
> respectively? Were they already there?

Yes, all of the bounded components have existing range checks.
However, those checks occur *after* the integer value of the component
has been read from the string.  While reading the integer from the
string, bounded components can overflow and then wrap back around
into their valid range.

> If so, reword the top of the commit message.

How's this?

The string to integer conversion predicates used by date_time_from_string/2
and duration_from_string/2 do not currently detect overflow and wrap around
while building up the integer values of date_time or duration components;
add the missing overflow checks.

> > +"23058430092136939520-01-01 00:00:00": bad overflow check
> > +PASS: ERROR DETECTED
> > diff --git a/tests/hard_coded/calendar_date_time_conv.m
> > b/tests/hard_coded/calendar_date_time_conv.m
> > index 994f5d788..a180d333f 100644
> > --- a/tests/hard_coded/calendar_date_time_conv.m
> > +++ b/tests/hard_coded/calendar_date_time_conv.m
> > @@ -181,7 +181,11 @@ valid_date_times = [
> >      dt_conv_test("all maximum", "2024-12-31 23:59:60.999999"),
> >      dt_conv_test("Unix epoch", "1970-01-01 00:00:00"),
> >      dt_conv_test("first day of the Gregorian calendar",
> > -        "1582-10-15 00:00:00")
> > +        "1582-10-15 00:00:00"),
> > +
> > +    % Valid in all grades.
> > +    dt_conv_test("year on 32-bit integer boundary",
> > +        "2147483647-01-01 00:00:00")
> >  ].
> >
> >      % These cannot be round-tripped through date_time_to_string/1.
> > @@ -294,7 +298,15 @@ invalid_date_times = [
> >      dt_conv_test("missing day", "2024-01- 00:00:00"),
> >      dt_conv_test("missing hour", "2024-01-01 :00:00"),
> >      dt_conv_test("missing minute", "2024-01-01 00::00"),
> > -    dt_conv_test("trailing colon with no second digits", "2024-01-01 00:00:")
> > +    dt_conv_test("trailing colon with no second digits", "2024-01-01 00:00:"),
> > +
> > +    % One past max_int for 64-bit integers.
> > +    dt_conv_test("integer overflow in year component",
> > +        "9223372036854775808-01-01 00:00:00"),
> > +
> > +    % A test case intended to defeat incorrect overflow checks.
> > +    dt_conv_test("bad overflow check",
> > +        "23058430092136939520-01-01 00:00:00")
> >  ].
>
> I don't know what kind of incorrectness you are checking for here,
> because I don't know what that huge integer is in relation to max_int
> for 64 bits.

It's a value that defeats the old incorrect overflow check that
string.to_int used.

...

> Wouldn't even 13 be an overflow in the months, 32 in the days component,
> etc?

Not in the sense of integer overflow.

> Are those tests missing?

Those tests are present.

Julien.