Re: Remove dependence on integer wrapping - Mailing list pgsql-hackers

>>               /* 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