Nathan Bossart <nathandbossart@gmail.com> writes:
> On Sun, Jun 09, 2024 at 04:59:22PM -0400, Joseph Koshakow wrote:
>> The following comment was in the code for parsing timestamps:
>> /* check for just-barely overflow (okay except time-of-day wraps) */
>> /* caution: we want to allow 1999-12-31 24:00:00 */
>>
>> I wasn't able to fully understand it even after staring at it for
>> a while. Is the comment suggesting that it is ok for the months field,
>> for example, to wrap around? That doesn't sound right to me I tested
>> the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same
>> before and after the patch.
> I haven't stared at this for a while like you, but I am likewise confused
> at first glance. This dates back to commit 84df54b, and it looks like this
> comment was present in the first version of the patch in the thread [0]. I
> CTRL+F'd for any discussion about this but couldn't immediately find
> anything.
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.
BTW, while I approve of trying to get rid of our need for -fwrapv,
I'm quite scared of actually doing it. Whatever cases you may have
discovered by running the regression tests will be at best the
tip of the iceberg. Is there any chance of using static analysis
to find all the places of concern?
regards, tom lane