Re: Remove dependence on integer wrapping

>> /* check the negative equivalent will fit without
overflowing */
>> if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
>> goto out_of_range;
>> +
>> + /*
>> + * special case the minimum integer because its negation
cannot be
>> + * represented
>> + */
>> + if (tmp == ((uint16) PG_INT16_MAX) + 1)
>> + return PG_INT16_MIN;
>> return -((int16) tmp);
>
> My first impression is that there appears to be two overflow checks, one
of
> which sends us to out_of_range, and another that just returns a special
> result. Why shouldn't we add a pg_neg_s16_overflow() and replace this
> whole chunk with something like this?
>
> if (unlikely(pg_neg_s16_overflow(tmp, &tmp)))
> goto out_of_range;
> else
> return tmp;

tmp is an uint16 here, it seems like you might have read it as an
int16? We would need some helper method like

static inline bool
pg_neg_u16_overflow(uint16 a, int16 *result);

and then we could replace that whole chunk with something like

if (unlikely(pg_neg_u16_overflow(tmp, &result)))
goto out_of_range;
else
return result;

that pattern shows up a lot in this file, but I was worried that it
wasn't useful as a general purpose function. Happy to add it
though if you still feel otherwise.

>> + return ((uint32) INT32_MAX) + 1;
>>
>> + return ((uint64) INT64_MAX) + 1;
>
> nitpick: Any reason not to use PG_INT32_MAX/PG_INT64_MAX for these?

Carelessness, sorry about that, it's been fixed in the attached patch.

>> I believe this is a copy-and-paste from 841b4a2d5, which added this:
>>
>> + *result = (date * INT64CONST(86400000000)) + time;
>> + /* check for major overflow */
>> + if ((*result - time) / INT64CONST(86400000000) != date)
>> + return -1;
>> + /* check for just-barely overflow (okay except time-of-day wraps) */
>> + if ((*result < 0) ? (date >= 0) : (date < 0))
>> + return -1;
>>
>> I think you could replace the whole thing by using overflow-aware
>> multiplication and addition primitives in the result calculation.
>> Lines 2-4 basically check for mult overflow and 5-7 for addition
>> overflow.
>
> Ah, I see. Joe's patch does that in one place. It's probably worth doing
> that in the other places this "just-barefly overflow" comment appears
IMHO.
>
> I was still confused by the comment about 1999, but I tracked it down to
>commit 542eeba [0]. IIUC it literally means that we need special handling
>for that date because POSTGRES_EPOCH_JDATE is 2000-01-01.
>
> [0]
https://postgr.es/m/CABUevEx5zUO%3DKRUg06a9qnQ_e9EvTKscL6HxAM_L3xj71R7AQw%40mail.gmail.com

> Yeah, I think so, and I think we probably don't need any special care
> if we switch to direct tests of overflow-aware primitives. (Though
>it'd be worth checking that '1999-12-31 24:00:00'::timestamp still
> works. It doesn't look like I actually added a test case for that.)

The only other place I found this comment was in
`make_timestamp_internal`. I've updated that function and added some
tests. I also manually verified that the behavior matches before and
after this patch.

>> BTW, while I approve of trying to get rid of our need for -fwrapv,
>> I'm quite scared of actually doing it.
>
> I think that's a quite fair concern. One potentially relevant datapoint is
> that we actually don't have -fwrapv equivalent on all platforms, and I
don't
>recall a lot of complaints from windows users. Of course it's quite
possible
> that they'd never notice...
>
> I think this is a good argument for enabling -ftrapv in development
> builds. That gives us at least a *chance* of seeing these issues.

+1, I wouldn't recommend removing -fwrapv immediately after this
commit. However, if we can enable -ftrapv in development builds, then
we can find overflows much more easily.

> Whatever cases you may have discovered by running the regression tests
will
> be at best the tip of the iceberg.

Agreed.

> Is there any chance of using static
> analysis to find all the places of concern?

I'm not personally familiar with any static analysis tools, but I can
try and do some research. Andres had previously suggested SQLSmith. I
think any kind of fuzz testing with -ftrapv enabled will reveal a lot
of issues. Honestly just grepping for +,-,* in certain directories
(like backend/utils/adt) would probably be fairly fruitful for anyone
with the patience. My previous overflow patch was the result of looking
through all the arithmetic in datetime.c.

Thanks,
Joe Koshakow