Horton

Thread

Commits

  1. Handle integer overflow in interval justification functions.

  1. Fix overflow in justify_interval related functions

    Joseph Koshakow <koshy44@gmail.com> — 2022-02-13T18:28:38Z

    I mentioned this issue briefly in another thread, but the
justify_interval, justify_days, and justify_hours functions
all have the potential to overflow. The attached patch fixes
this issue.

Cheers,
Joe Koshakow

  2. Re: Fix overflow in justify_interval related functions

    Nathan Bossart <nathandbossart@gmail.com> — 2022-02-14T18:35:40Z

    On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:
> +SELECT justify_hours(interval '2147483647 days 24 hrs');
> +ERROR:  interval out of range

The docs [0] claim that the maximum value for interval is 178 million
years, but this test case is only ~6 million.  Should we instead rework the
logic to avoid overflow for this case?

[0] https://www.postgresql.org/docs/devel/datatype-datetime.html

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  3. Re: Fix overflow in justify_interval related functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-14T18:55:56Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:
>> +SELECT justify_hours(interval '2147483647 days 24 hrs');
>> +ERROR:  interval out of range

> The docs [0] claim that the maximum value for interval is 178 million
> years, but this test case is only ~6 million.  Should we instead rework the
> logic to avoid overflow for this case?

I think the docs are misleading you on this point.  The maximum
value of the months part of an interval is 2^31 months or
about 178Myr, but what we're dealing with here is days, which
likewise caps at 2^31 days.  justify_hours is not chartered
to transpose up to months, so it can't avoid that limit.

			regards, tom lane

  4. Re: Fix overflow in justify_interval related functions

    Nathan Bossart <nathandbossart@gmail.com> — 2022-02-14T19:15:09Z

    On Mon, Feb 14, 2022 at 01:55:56PM -0500, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:
>>> +SELECT justify_hours(interval '2147483647 days 24 hrs');
>>> +ERROR:  interval out of range
> 
>> The docs [0] claim that the maximum value for interval is 178 million
>> years, but this test case is only ~6 million.  Should we instead rework the
>> logic to avoid overflow for this case?
> 
> I think the docs are misleading you on this point.  The maximum
> value of the months part of an interval is 2^31 months or
> about 178Myr, but what we're dealing with here is days, which
> likewise caps at 2^31 days.  justify_hours is not chartered
> to transpose up to months, so it can't avoid that limit.

Makes sense.  So we could likely avoid it for justify_interval, but the
others are at the mercy of the interval implementation.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  5. Re: Fix overflow in justify_interval related functions

    Joseph Koshakow <koshy44@gmail.com> — 2022-02-14T21:57:07Z

    On Mon, Feb 14, 2022 at 2:15 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Makes sense.  So we could likely avoid it for justify_interval, but the
> others are at the mercy of the interval implementation.

I'm not entirely sure what you mean by "it", but for both
justify_interval and justify_days this commit throws an error if we
try to set the months field above 2^31.

> +SELECT justify_days(interval '2147483647 months 30 days');
> +ERROR:  interval out of range
> +SELECT justify_interval(interval '2147483647 months 30 days');
> +ERROR:  interval out of range

That's what these are testing.

- Joe Koshakow

  6. Re: Fix overflow in justify_interval related functions

    Nathan Bossart <nathandbossart@gmail.com> — 2022-02-15T00:59:18Z

    On Mon, Feb 14, 2022 at 04:57:07PM -0500, Joseph Koshakow wrote:
> On Mon, Feb 14, 2022 at 2:15 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Makes sense.  So we could likely avoid it for justify_interval, but the
>> others are at the mercy of the interval implementation.
> 
> I'm not entirely sure what you mean by "it", but for both
> justify_interval and justify_days this commit throws an error if we
> try to set the months field above 2^31.
> 
>> +SELECT justify_days(interval '2147483647 months 30 days');
>> +ERROR:  interval out of range
>> +SELECT justify_interval(interval '2147483647 months 30 days');
>> +ERROR:  interval out of range
> 
> That's what these are testing.

I think it's possible to avoid overflow in justify_interval() in some cases
by pre-justifying the days.  I've attached a patch to demonstrate.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  7. Re: Fix overflow in justify_interval related functions

    Joseph Koshakow <koshy44@gmail.com> — 2022-02-15T01:35:43Z

    On Mon, Feb 14, 2022 at 7:59 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I think it's possible to avoid overflow in justify_interval() in some cases
> by pre-justifying the days.  I've attached a patch to demonstrate.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com

Good catch, I didn't think about that. Though if you are pre-justifying
the days, then I don't think it's possible for the second addition to
days to overflow. The maximum amount the days field could be after
the first justification is 29. I think we can simplify it further to the
attached patch.

- Joe

  8. Re: Fix overflow in justify_interval related functions

    Nathan Bossart <nathandbossart@gmail.com> — 2022-02-15T04:23:06Z

    On Mon, Feb 14, 2022 at 08:35:43PM -0500, Joseph Koshakow wrote:
> Good catch, I didn't think about that. Though if you are pre-justifying
> the days, then I don't think it's possible for the second addition to
> days to overflow. The maximum amount the days field could be after
> the first justification is 29. I think we can simplify it further to the
> attached patch.

LGTM.  I've marked this one as ready-for-committer.  It's a little weird
that justify_hours() and justify_days() can overflow in cases where there
is still a valid interval representation, but as Tom noted, those functions
have specific charters to follow.

> +		ereport(ERROR,
> +				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> +				errmsg("interval out of range")));

nitpick: I think there is ordinarily an extra space before errmsg() so that
it lines up with errcode().

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  9. Re: Fix overflow in justify_interval related functions

    Joseph Koshakow <koshy44@gmail.com> — 2022-02-15T11:42:23Z

    On Mon, Feb 14, 2022 at 11:23 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> It's a little weird
> that justify_hours() and justify_days() can overflow in cases where there
> is still a valid interval representation, but as Tom noted, those functions
> have specific charters to follow.

Yes it is a bit weird, but this follows the same behavior as adding
Intervals. The following query overflows:
  postgres=# SELECT interval '2147483647 days' + interval '1 day';
  ERROR:  interval out of range
Even though the following query does not:
  postgres=# SELECT justify_days(interval '2147483647 days') + interval '1 day';
            ?column?
  -----------------------------
   5965232 years 4 mons 8 days
  (1 row)

The reason is, as Tom mentioned, that Interval's months, days, and
time (microseconds) are stored separately. They are only combined
during certain scenarios such as testing for equality, ordering,
justify_* methods, etc. I think the idea behind it is that not every month
has 30 days and not every day has 24 hrs, though I'm not sure
.
> > +             ereport(ERROR,
> > +                             (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> > +                             errmsg("interval out of range")));
>
> nitpick: I think there is ordinarily an extra space before errmsg() so that
> it lines up with errcode().

I've attached a patch to add the space. Thanks so much for your review
and comments!

- Joe Koshakow

  10. Re: Fix overflow in justify_interval related functions

    Joseph Koshakow <koshy44@gmail.com> — 2022-02-25T15:30:57Z

    Just checking because I'm not very familiar with the process,
are there any outstanding items that I need to do for this patch?

- Joe Koshakow

  11. Re: Fix overflow in justify_interval related functions

    Nathan Bossart <nathandbossart@gmail.com> — 2022-02-25T19:04:35Z

    On Fri, Feb 25, 2022 at 10:30:57AM -0500, Joseph Koshakow wrote:
> Just checking because I'm not very familiar with the process,
> are there any outstanding items that I need to do for this patch?

Unless someone has additional feedback, I don't think so.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  12. Re: Fix overflow in justify_interval related functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-28T20:38:31Z

    Joseph Koshakow <koshy44@gmail.com> writes:
> [ v4-0001-Check-for-overflow-in-justify_interval-functions.patch ]

Pushed.  I added a comment explaining why the one addition in
interval_justify_interval doesn't require an overflow check.

			regards, tom lane