Обсуждение: Remove dependence on integer wrapping
Hi,
In [0] Andres suggested enabling -ftrapv in assert enabled builds. While
I vastly underestimated the complexity of updating `configure` to do
this, I was able to enable the flag locally. Enabling this flag causes
our existing regression tests to trap and fail in multiple different
spots. The attached patch resolves all of these overflows so that all
of our existing tests will pass with the -ftrapv flag enabled.
Some notes on the patch itself are:
I originally added the helper functions to int.h thinking I'd find
many more instances of overflow due to integer negation, however I
didn't find that many. So let me know if you think we'd be better
off without the functions.
I considered using #ifdef to rely on wrapping when -fwrapv was
enabled. This would save us some unnecessary branching when we could
rely on wrapping behavior, but it would mean that we could only enable
-ftrapv when -fwrapv was disabled, greatly reducing its utility.
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.
Thanks,
Joe Koshakow
[0] https://www.postgresql.org/message-id/20240213191401.jjhsic7et4tiahjs%40awork3.anarazel.de
In [0] Andres suggested enabling -ftrapv in assert enabled builds. While
I vastly underestimated the complexity of updating `configure` to do
this, I was able to enable the flag locally. Enabling this flag causes
our existing regression tests to trap and fail in multiple different
spots. The attached patch resolves all of these overflows so that all
of our existing tests will pass with the -ftrapv flag enabled.
Some notes on the patch itself are:
I originally added the helper functions to int.h thinking I'd find
many more instances of overflow due to integer negation, however I
didn't find that many. So let me know if you think we'd be better
off without the functions.
I considered using #ifdef to rely on wrapping when -fwrapv was
enabled. This would save us some unnecessary branching when we could
rely on wrapping behavior, but it would mean that we could only enable
-ftrapv when -fwrapv was disabled, greatly reducing its utility.
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.
Thanks,
Joe Koshakow
[0] https://www.postgresql.org/message-id/20240213191401.jjhsic7et4tiahjs%40awork3.anarazel.de
Вложения
On Sun, Jun 09, 2024 at 04:59:22PM -0400, Joseph Koshakow wrote:
> I originally added the helper functions to int.h thinking I'd find
> many more instances of overflow due to integer negation, however I
> didn't find that many. So let me know if you think we'd be better
> off without the functions.
I'd vote for the functions, even if it's just future-proofing at the
moment. IMHO it helps with readability, too.
> 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.
> /* 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;
> + return ((uint32) INT32_MAX) + 1;
> + return ((uint64) INT64_MAX) + 1;
nitpick: Any reason not to use PG_INT32_MAX/PG_INT64_MAX for these?
[0] https://postgr.es/m/flat/CAFj8pRBwqALkzc%3D1WV%2Bh5e%2BDcALY2EizjHCvFi9vHbs%2Bz1OhjA%40mail.gmail.com
--
nathan
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
On Sun, Jun 09, 2024 at 09:57:54PM -0400, Tom Lane wrote: > 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. 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 -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Sun, Jun 09, 2024 at 09:57:54PM -0400, Tom Lane wrote:
>> I think you could replace the whole thing by using overflow-aware
>> multiplication and addition primitives in the result calculation.
> 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.
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.)
regards, tom lane
Hi, On 2024-06-09 21:57:54 -0400, Tom Lane wrote: > 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. > 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? The last time I tried removing -fwrapv both gcc and clang found quite a few issues. I think I fixed most of those though, so presumably the issue that remain are ones less easily found by simple static analysis. I wonder if coverity would find more if we built without -fwrapv. GCC has -Wstrict-overflow=n, which at least tells us where the compiler optimizes based on the assumption that there cannot be overflow. It does generate a bunch of noise, but I think it's almost exclusively due to our MemSet() macro. Greetings, Andres Freund
>> /* 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
>> 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
Вложения
On Tue, Jun 11, 2024 at 09:31:39AM -0400, Joseph Koshakow wrote: > 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. I personally find that much easier to read. Since the existing open-coded overflow check is apparently insufficient, I think there's a reasonably strong case for centralizing this sort of thing so that we don't continue to make the same mistakes. >> 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. > > 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. tm2timestamp() in src/interfaces/ecpg/pgtypeslib/timestamp.c has the same comment. The code there looks very similar to the code for tm2timestamp() in the other timestamp.c... -- nathan
On Tue, Jun 11, 2024 at 12:22 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I personally find that much easier to read. Since the existing open-coded
> overflow check is apparently insufficient, I think there's a reasonably
> strong case for centralizing this sort of thing so that we don't continue
> to make the same mistakes.
Sounds good, the attached patch has these changes.
> tm2timestamp() in src/interfaces/ecpg/pgtypeslib/timestamp.c has the same
> comment. The code there looks very similar to the code for tm2timestamp()
> in the other timestamp.c...
The attached patch has updated this file too. For some reason I was
under the impression that I should leave the ecpg/ files alone, though
I can't remember why.
Thanks,
Joe Koshakow
> I personally find that much easier to read. Since the existing open-coded
> overflow check is apparently insufficient, I think there's a reasonably
> strong case for centralizing this sort of thing so that we don't continue
> to make the same mistakes.
Sounds good, the attached patch has these changes.
> tm2timestamp() in src/interfaces/ecpg/pgtypeslib/timestamp.c has the same
> comment. The code there looks very similar to the code for tm2timestamp()
> in the other timestamp.c...
The attached patch has updated this file too. For some reason I was
under the impression that I should leave the ecpg/ files alone, though
I can't remember why.
Thanks,
Joe Koshakow
Вложения
On Tue, Jun 11, 2024 at 09:10:44PM -0400, Joseph Koshakow wrote: > The attached patch has updated this file too. For some reason I was > under the impression that I should leave the ecpg/ files alone, though > I can't remember why. Thanks. This looks pretty good to me after a skim, so I'll see about committing/back-patching it in the near future. IIUC there is likely more to come in this area, but I see no need to wait. (I'm chuckling that we are adding Y2K tests in 2024...) -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> Thanks. This looks pretty good to me after a skim, so I'll see about
> committing/back-patching it in the near future. IIUC there is likely more
> to come in this area, but I see no need to wait.
Uh ... what of this would we back-patch? It seems like v18
material to me.
regards, tom lane
On Wed, Jun 12, 2024 at 11:45:20AM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> Thanks. This looks pretty good to me after a skim, so I'll see about >> committing/back-patching it in the near future. IIUC there is likely more >> to come in this area, but I see no need to wait. > > Uh ... what of this would we back-patch? It seems like v18 > material to me. D'oh. I was under the impression that the numutils.c changes were arguably bug fixes. Even in that case, we should probably split out the other stuff for v18. But you're probably right that we could just wait for all of it. -- nathan
On Mon, Jun 10, 2024 at 2:30 PM Andres Freund <andres@anarazel.de> wrote: > On 2024-06-09 21:57:54 -0400, Tom Lane wrote: > > BTW, while I approve of trying to get rid of our need for -fwrapv, > > I'm quite scared of actually doing it. IMV it's perfectly fine to defensively assume that we need -fwrapv, even given a total lack of evidence that removing it would cause harm. Doing it to "be more in line with the standard" is a terrible argument. > 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. That might just be because MSVC inherently doesn't do optimizations that rely on signed wrap being undefined behavior. Last I checked MSVC just doesn't rely on the standard's strict aliasing rules, even as an opt-in thing. > Of course it's quite possible > that they'd never notice... Removing -fwrapv is something that I see as a potential optimization. It should be justified in about the same way as any other optimization. I suspect that it doesn't actually have any clear benefits for us. But if I'm wrong about that then the benefit might still be achievable through other means (something far short of just removing -fwrapv globally). > 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'm definitely prepared to say that code that actively relies on -fwrapv is broken code. -- Peter Geoghegan
10.06.2024 04:57, Tom Lane wrote:
> 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?
Let me remind you of bug #18240. Yes, that was about float8, but with
-ftrapv we can get into the trap with:
SELECT 1_000_000_000::money * 1_000_000_000::int;
server closed the connection unexpectedly
Also there are several trap-producing cases with date types:
SELECT to_date('100000000', 'CC');
SELECT to_timestamp('1000000000,999', 'Y,YYY');
SELECT make_date(-2147483648, 1, 1);
And one more with array...
CREATE TABLE t (ia int[]);
INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');
I think it's not the whole iceberg too.
Best regards,
Alexander
On Thu, Jun 13, 2024 at 12:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> Let me remind you of bug #18240. Yes, that was about float8, but with
> -ftrapv we can get into the trap with:
> SELECT 1_000_000_000::money * 1_000_000_000::int;
> server closed the connection unexpectedly
Interesting, it looks like there's no overflow handling of any money
arithmetic. I've attached
v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
overflow checks and tests. I didn't address the float multiplication
because I didn't see any helper methods in int.h. I did some some
useful helpers in float.h, but they raise an error directly instead
of returning a bool. Would those be appropriate for use with the
money type? If not I can refactor out the inner parts into a new method
that returns a bool.
v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
just incremented the version number.
> Also there are several trap-producing cases with date types:
> SELECT to_date('100000000', 'CC');
> SELECT to_timestamp('1000000000,999', 'Y,YYY');
> SELECT make_date(-2147483648, 1, 1);
>
> And one more with array...
> CREATE TABLE t (ia int[]);
> INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');
I'll try and get patches to address these too in the next couple of
weeks unless someone beats me to it.
> I think it's not the whole iceberg too.
+1
Thanks,
Joe Koshakow
>
> Let me remind you of bug #18240. Yes, that was about float8, but with
> -ftrapv we can get into the trap with:
> SELECT 1_000_000_000::money * 1_000_000_000::int;
> server closed the connection unexpectedly
Interesting, it looks like there's no overflow handling of any money
arithmetic. I've attached
v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
overflow checks and tests. I didn't address the float multiplication
because I didn't see any helper methods in int.h. I did some some
useful helpers in float.h, but they raise an error directly instead
of returning a bool. Would those be appropriate for use with the
money type? If not I can refactor out the inner parts into a new method
that returns a bool.
v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
just incremented the version number.
> Also there are several trap-producing cases with date types:
> SELECT to_date('100000000', 'CC');
> SELECT to_timestamp('1000000000,999', 'Y,YYY');
> SELECT make_date(-2147483648, 1, 1);
>
> And one more with array...
> CREATE TABLE t (ia int[]);
> INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');
I'll try and get patches to address these too in the next couple of
weeks unless someone beats me to it.
> I think it's not the whole iceberg too.
+1
Thanks,
Joe Koshakow
Вложения
On Thu, Jun 13, 2024 at 10:48 PM Joseph Koshakow <koshy44@gmail.com> wrote:
> I've attached
> v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
> overflow checks and tests. I didn't address the float multiplication
> because I didn't see any helper methods in int.h. I did some some
> useful helpers in float.h, but they raise an error directly instead
> of returning a bool. Would those be appropriate for use with the
> money type? If not I can refactor out the inner parts into a new method
> that returns a bool.
> v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
> just incremented the version number.
Oops I left a careless mistake in that last patch, my apologies. It's
> I've attached
> v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
> overflow checks and tests. I didn't address the float multiplication
> because I didn't see any helper methods in int.h. I did some some
> useful helpers in float.h, but they raise an error directly instead
> of returning a bool. Would those be appropriate for use with the
> money type? If not I can refactor out the inner parts into a new method
> that returns a bool.
> v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
> just incremented the version number.
Oops I left a careless mistake in that last patch, my apologies. It's
fixed in the attached patches.
Thanks,
Joe Koshakow
Вложения
14.06.2024 05:48, Joseph Koshakow wrote:
>
> v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
> just incremented the version number.
>
> > Also there are several trap-producing cases with date types:
> > SELECT to_date('100000000', 'CC');
> > SELECT to_timestamp('1000000000,999', 'Y,YYY');
> > SELECT make_date(-2147483648, 1, 1);
> >
> > And one more with array...
> > CREATE TABLE t (ia int[]);
> > INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');
>
> I'll try and get patches to address these too in the next couple of
> weeks unless someone beats me to it.
>
> > I think it's not the whole iceberg too.
>
> +1
After sending my message, I toyed with -ftrapv a little time more and
found other cases:
SELECT '[]'::jsonb -> -2147483648;
#4 0x00007efe232d67f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x000055e8fde9f211 in __negvsi2 ()
#6 0x000055e8fdcca62c in jsonb_array_element (fcinfo=0x55e8fec28220) at jsonfuncs.c:948
(gdb) f 6
#6 0x000055e14cb9362c in jsonb_array_element (fcinfo=0x55e14d493220) at jsonfuncs.c:948
948 if (-element > nelements)
(gdb) p element
$1 = -2147483648
---
SELECT jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
#4 0x00007f1873bef7f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x0000564a009d2211 in __negvsi2 ()
#6 0x0000564a00807c89 in setPathArray (it=0x7fff865c7380, path_elems=0x564a017baf20, path_nulls=0x564a017baf40,
path_len=2, st=0x7fff865c7388, level=1, newval=0x0, nelems=2, op_type=2) at jsonfuncs.c:5407
(gdb) f 6
#6 0x000055985e823c89 in setPathArray (it=0x7ffc22258fe0, path_elems=0x559860286f20, path_nulls=0x559860286f40,
path_len=2, st=0x7ffc22258fe8, level=1, newval=0x0, nelems=0, op_type=2) at jsonfuncs.c:5407
5407 if (-idx > nelems)
(gdb) p idx
$1 = -2147483648
---
CREATE FUNCTION check_foreign_key () RETURNS trigger AS .../refint.so' LANGUAGE C;
CREATE TABLE t (i int4 NOT NULL);
CREATE TRIGGER check_fkey BEFORE DELETE ON t FOR EACH ROW EXECUTE PROCEDURE
check_foreign_key (2147483647, 'cascade', 'i', "ft", "i");
INSERT INTO t VALUES (1);
DELETE FROM t;
#4 0x00007f57f0bef7f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x00007f57f1671351 in __addvsi3 () from .../src/test/regress/refint.so
#6 0x00007f57f1670234 in check_foreign_key (fcinfo=0x7ffebf523650) at refint.c:321
(gdb) f 6
#6 0x00007f3400ef9234 in check_foreign_key (fcinfo=0x7ffd6e16a600) at refint.c:321
321 nkeys = (nargs - nrefs) / (nrefs + 1);
(gdb) p nargs
$1 = 3
(gdb) p nrefs
$2 = 2147483647
---
And the most interesting case to me:
SET temp_buffers TO 1000000000;
CREATE TEMP TABLE t(i int PRIMARY KEY);
INSERT INTO t VALUES(1);
#4 0x00007f385cdd37f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x00005620071c4f51 in __addvsi3 ()
#6 0x0000562007143f3c in init_htab (hashp=0x562008facb20, nelem=610070812) at dynahash.c:720
(gdb) f 6
#6 0x0000560915207f3c in init_htab (hashp=0x560916039930, nelem=1000000000) at dynahash.c:720
720 hctl->high_mask = (nbuckets << 1) - 1;
(gdb) p nbuckets
$1 = 1073741824
Best regards,
Alexander
On Thu, Jun 13, 2024 at 10:56 PM Joseph Koshakow <koshy44@gmail.com> wrote:
On Thu, Jun 13, 2024 at 10:48 PM Joseph Koshakow <koshy44@gmail.com> wrote:
> I've attached
> v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
> overflow checks and tests. I didn't address the float multiplication
> because I didn't see any helper methods in int.h. I did some some
> useful helpers in float.h, but they raise an error directly instead
> of returning a bool. Would those be appropriate for use with the
> money type? If not I can refactor out the inner parts into a new method
> that returns a bool.
> v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
> just incremented the version number.
I added overflow handling for float arithmetic to the `money` type.
v6-0002-Handle-overflow-in-money-arithmetic.patch is ready for review.
v6-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
just incremented the version number.
Thanks,
Joe Koshakow
On Thu, Jun 13, 2024 at 10:48 PM Joseph Koshakow <koshy44@gmail.com> wrote:
> I've attached
> v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
> overflow checks and tests. I didn't address the float multiplication
> because I didn't see any helper methods in int.h. I did some some
> useful helpers in float.h, but they raise an error directly instead
> of returning a bool. Would those be appropriate for use with the
> money type? If not I can refactor out the inner parts into a new method
> that returns a bool.
> v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
> just incremented the version number.
I added overflow handling for float arithmetic to the `money` type.
v6-0002-Handle-overflow-in-money-arithmetic.patch is ready for review.
v6-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
just incremented the version number.
Thanks,
Joe Koshakow