Re: INTERVAL overflow detection is terribly broken

On Sun, Jan 26, 2014 at 02:13:03PM +0100, Florian Pflug wrote:
> On Jan26, 2014, at 03:50 , Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Patch attached.
>
> > + if ((float)tm->tm_year * MONTHS_PER_YEAR + tm->tm_mon > INT_MAX)
> > + return -1;
>
> Is this bullet-proof? If float and int are both 32-bit, the float's mantissa
> will be less than 32-bit (24 or so, I think), and thus won't be able to
> represent INT_MAX accurately. This means there's a value of
> tm_year * MONTHS_PER_YEAR which is less than INT_MAX, yet for which
> tm_year * MONTHS_PER_YEAR + tm_mon will return tm_year * MONTHS_PER_YEAR
> unmodified if tm_mon is small enough (e.g, 1). But if tm_year * MONTHS_PER_YEAR
> was close enough to INT_MAX, the actual value of
> tm_year * MONTHS_PER_YEAR + tm_mon might still be larger than INT_MAX,
> the floating-point computation just won't detect it. In that case, the
> check would succeed, yet the actual integer computation would still overflow.
>
> It should be safe with "double" instead of "float".

You are correct. I have adjusted the code to use "double".

> > + if (SAMESIGN(span1->month, span2->month) &&
> > + !SAMESIGN(result->month, span1->month))
>
> This assumes wrapping semantics for signed integral types, which isn't
> guaranteed by the C standard - it says overflows of signed integral types
> produce undefined results. We currently depend on wrapping semantics for
> these types in more places, and therefore need GCC's "-fwrapv" anway, but
> I still wonder if adding more of these kinds of checks is a good idea.

Well, I copied this from int.c, so what I did was to mention the other
function I got this code from. If we ever change int.c we would need to
change this too.

The updated attached patch has overflow detection for interval
subtraction, multiply, and negate. There are also some comparison
cleanups.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +