Remove dependence on integer wrapping

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]/messages/by-id/flat/CAFj8pRBwqALkzc=1WV+h5e+DcALY2EizjHCvFi9vHbs+z1OhjA@mail.gmail.com. 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]: /messages/by-id/flat/CAFj8pRBwqALkzc=1WV+h5e+DcALY2EizjHCvFi9vHbs+z1OhjA@mail.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]/messages/by-id/CABUevEx5zUO=KRUg06a9qnQ_e9EvTKscL6HxAM_L3xj71R7AQw@mail.gmail.com. IIUC it literally means that we need special handling
for that date because POSTGRES_EPOCH_JDATE is 2000-01-01.

[0]: /messages/by-id/CABUevEx5zUO=KRUg06a9qnQ_e9EvTKscL6HxAM_L3xj71R7AQw@mail.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]

/messages/by-id/CABUevEx5zUO=KRUg06a9qnQ_e9EvTKscL6HxAM_L3xj71R7AQw@mail.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

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

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
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 12:00 AM Alexander Lakhin <exclusion@gmail.com>
wrote:

And one more with array...
CREATE TABLE t (ia int[]);
INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');

I've added another patch, 0003, to resolve this wrap-around. In fact I
discovered a bug that the following statement is accepted and inserts
an empty array into the table.

INSERT INTO t(ia[-2147483648:2147483647]) VALUES ('{}');

My patch resolves this bug as well.

The other patches, 0001 and 0002, are unchanged but have their version
number incremented.

As a reminder, 0001 is reviewed and waiting for v18 and a committer.
0002 and 0003 are unreviewed. So, I'm going to mark this as waiting for
a reviewer.

Thanks,
Joe Koshakow

On Thu, Jun 13, 2024 at 12:00 AM Alexander Lakhin <exclusion@gmail.com>
wrote:

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

I've added another patch, 0004, to resolve the jsonb wrap-arounds.

The other patches, 0001, 0002, and 0003 are unchanged but have their
version number incremented.

Thanks,
Joe Koshakow