Fix overflow hazard in interval rounding

Joseph Koshakow <koshy44@gmail.com> writes:

Attached is a patch that fixes some overflow/underflow hazards that I
discovered in the interval rounding code.

I think you need to use ereturn not ereport here; see other error
cases in AdjustIntervalForTypmod.

(We'd need ereport in back branches, but this problem seems to
me to probably not be worth back-patching.)

regards, tom lane

Hi,

On 2024-02-13 13:31:22 -0500, Joseph Koshakow wrote:

Attached is a patch that fixes some overflow/underflow hazards that I
discovered in the interval rounding code.

Random, mildly related thought: I wonder if it's time to, again, look at
enabling -ftrapv in assert enabled builds. I had looked at that a few years
back, and fixed a number of instances, but not all I think. But I think we are
a lot closer to avoiding signed overflows everywhere, and it'd be nice to find
overflow hazards more easily. Many places are broken even with -fwrapv
semantics (which we don't have on all compilers!). Trapping on such overflows
makes it far easier to find problems with tools like sqlsmith.

Greetings,

Andres Freund

On Tue, Feb 13, 2024 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think you need to use ereturn not ereport here; see other error
cases in AdjustIntervalForTypmod.

Attached is an updated patch that makes this adjustment.

(We'd need ereport in back branches, but this problem seems to
me to probably not be worth back-patching.)

Agreed, this seems like a pretty rare overflow/underflow.

Thanks,
Joe Koshakow

Joseph Koshakow <koshy44@gmail.com> writes:

On Tue, Feb 13, 2024 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

(We'd need ereport in back branches, but this problem seems to
me to probably not be worth back-patching.)

Agreed, this seems like a pretty rare overflow/underflow.

OK, pushed to HEAD only. I converted the second steps to be like
"a -= a%b" instead of "a = (a/b)*b" to make it a little clearer
that they don't have their own risks of overflow. Maybe it's a
shade faster that way too, not sure.

regards, tom lane

Hi Andres,

Sorry for such a late reply.

On Tue, Feb 13, 2024 at 2:14 PM Andres Freund <andres@anarazel.de> wrote:

Random, mildly related thought: I wonder if it's time to, again, look at
enabling -ftrapv in assert enabled builds.I had looked at that a few years
back, and fixed a number of instances, but not all I think. But I think

we are

a lot closer to avoiding signed overflows everywhere, and it'd be nice to

find

overflow hazards more easily.

I agree that this would be very helpful.

Many places are broken even with -fwrapv
semantics (which we don't have on all compilers!). Trapping on such

overflows

makes it far easier to find problems with tools like sqlsmith.

Does this mean that some of our existing tests will panic when compiled
with -ftrapv or -fwrapv? If so I'd be interested in resolving the
remaining issues if you could point me in the right direction of how to
set the flag.

Thanks,
Joe Koshakow