Re: Fix overflow in DecodeInterval

On Fri, Feb 11, 2022 at 4:58 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> > Joseph Koshakow <koshy44(at)gmail(dot)com> writes:
> > > The attached patch fixes an overflow bug in DecodeInterval when applying
> > > the units week, decade, century, and millennium. The overflow check logic
> > > was modelled after the overflow check at the beginning of `int
> > > tm2interval(struct pg_tm *tm, fsec_t fsec, Interval *span);` in timestamp.c.
> >
> >
> > Good catch, but I don't think that tm2interval code is best practice
> > anymore.  Rather than bringing "double" arithmetic into the mix,
> > you should use the overflow-detecting arithmetic functions in
> > src/include/common/int.h.  The existing code here is also pretty
> > faulty in that it doesn't notice addition overflow when combining
> > multiple units.  So for example, instead of
> >
> >
> >     tm->tm_mday += val * 7;
> >
> >
> > I think we should write something like
> >
> >
> >     if (pg_mul_s32_overflow(val, 7, &tmp))
> >         return DTERR_FIELD_OVERFLOW;
> >     if (pg_add_s32_overflow(tm->tm_mday, tmp, &tm->tm_mday))
> >         return DTERR_FIELD_OVERFLOW;
> >
> >
> > Perhaps some macros could be used to make this more legible?
> >
> >
> >             regards, tom lane
> >
> >
> >     @postgresql
>
> Thanks for the feedback Tom, I've attached an updated patch with
> your suggestions. Feel free to rename the horribly named macro.
>
> Also while fixing this I noticed that fractional intervals can also
> cause an overflow issue.
> postgres=# SELECT INTERVAL '0.1 months 2147483647 days';
>      interval
> ------------------
>  -2147483646 days
> (1 row)
> I haven't looked into it, but it's probably a similar cause.

Hey Tom,

I was originally going to fix the fractional issue in a follow-up
patch, but I took a look at it and decided to make a new patch
with both fixes. I tried to make the handling of fractional and
non-fractional units consistent. I've attached the patch below.

While investigating this, I've noticed a couple more overflow
issues with Intervals, but I think they're best handled in separate
patches. I've included the ones I've found in this email.

  postgres=# CREATE TEMP TABLE INTERVAL_TBL_OF (f1 interval);
  CREATE TABLE
  postgres=# INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 weeks
2147483647 hrs');
  INSERT 0 1
  postgres=# SELECT * FROM INTERVAL_TBL_OF;
  ERROR:  interval out of range

  postgres=# SELECT justify_days(INTERVAL '2147483647 months 2147483647 days');
             justify_days
  -----------------------------------
   -172991738 years -4 mons -23 days
  (1 row)

Cheers,
Joe Koshakow