Thread: BUG #17888: Incorrect memory access in gist__int_ops for an input array with many elements

The following bug has been logged on the website:

Bug reference:      17888
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 15.2
Operating system:   Ubuntu 22.04
Description:

The following script:
CREATE EXTENSION intarray;
CREATE TABLE test__int(a int[]);
CREATE INDEX test_idx on test__int using gist (a gist__int_ops);
INSERT INTO test__int SELECT array(SELECT x FROM generate_series(1, 1001) x)
FROM generate_series(1, 10);

causes a server crash:
CREATE INDEX
NOTICE:  input array is too big (199 maximum allowed, 1001 current), use
gist__intbig_ops opclass instead
NOTICE:  input array is too big (199 maximum allowed, 1001 current), use
gist__intbig_ops opclass instead
NOTICE:  input array is too big (199 maximum allowed, 1001 current), use
gist__intbig_ops opclass instead
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.

with the stack trace:
Core was generated by `postgres: law regression [local] INSERT
                        '.
Program terminated with signal SIGSEGV, Segmentation fault.

warning: Section `.reg-xstate/2702527' in core file too small.
#0  0x00005610b8d00aa3 in GetMemoryChunkContext (pointer=0x7ff21ceac7b0)
    at ../../../../src/include/utils/memutils.h:131
131             AssertArg(MemoryContextIsValid(context));
(gdb) bt
#0  0x00005610b8d00aa3 in GetMemoryChunkContext (pointer=0x7ff21ceac7b0)
    at ../../../../src/include/utils/memutils.h:131
#1  0x00005610b8d02ea7 in pfree (pointer=0x7ff21ceac7b0) at mcxt.c:1177
#2  0x00007ff263238bb4 in g_int_decompress (fcinfo=0x7ffe00cda520) at
_int_gist.c:345
#3  0x00005610b8cc93f2 in FunctionCall1Coll (flinfo=0x5610ba16cf08,
collation=100, arg1=94629841722496) at fmgr.c:1124
#4  0x00005610b85c499e in gistdentryinit (giststate=0x5610ba16bce0, nkey=0,
e=0x5610ba19dc80, k=140677843961776, 
    r=0x7ff21a1a80b8, pg=0x7ff21ceab780 "", o=1, l=false, isNull=false) at
gistutil.c:562
#5  0x00005610b85c1fd1 in gistSplitByKey (r=0x7ff21a1a80b8,
page=0x7ff21ceab780 "", itup=0x5610ba19dc20, len=3, 
    giststate=0x5610ba16bce0, v=0x7ffe00cda750, attno=0) at
gistsplit.c:644
#6  0x00005610b85b39ca in gistSplit (r=0x7ff21a1a80b8, page=0x7ff21ceab780
"", itup=0x5610ba19dc20, len=3, 
    giststate=0x5610ba16bce0) at gist.c:1450
#7  0x00005610b85af946 in gistplacetopage (rel=0x7ff21a1a80b8, freespace=0,
giststate=0x5610ba16bce0, buffer=1804, 
    itup=0x7ffe00cdac00, ntup=1, oldoffnum=0, newblkno=0x0, leftchildbuf=0,
splitinfo=0x7ffe00cdabb0, 
    markfollowright=true, heapRel=0x7ff21a1acef0, is_build=false) at
gist.c:309
#8  0x00005610b85b34c5 in gistinserttuples (state=0x7ffe00cdacc0,
stack=0x7ffe00cdac90, giststate=0x5610ba16bce0, 
    tuples=0x7ffe00cdac00, ntup=1, oldoffnum=0, leftchild=0, rightchild=0,
unlockbuf=false, unlockleftchild=false)
    at gist.c:1278
#9  0x00005610b85b33f8 in gistinserttuple (state=0x7ffe00cdacc0,
stack=0x7ffe00cdac90, giststate=0x5610ba16bce0, 
    tuple=0x5610ba19cbe0, oldoffnum=0) at gist.c:1231
#10 0x00005610b85b1f1e in gistdoinsert (r=0x7ff21a1a80b8,
itup=0x5610ba19cbe0, freespace=0, giststate=0x5610ba16bce0, 
    heapRel=0x7ff21a1acef0, is_build=false) at gist.c:886
#11 0x00005610b85af35c in gistinsert (r=0x7ff21a1a80b8,
values=0x7ffe00cdae80, isnull=0x7ffe00cdae60, 
    ht_ctid=0x5610ba15e510, heapRel=0x7ff21a1acef0,
checkUnique=UNIQUE_CHECK_NO, indexUnchanged=false, 
    indexInfo=0x5610ba15f9c8) at gist.c:183
#12 0x00005610b8615242 in index_insert (indexRelation=0x7ff21a1a80b8,
values=0x7ffe00cdae80, isnull=0x7ffe00cdae60, 
    heap_t_ctid=0x5610ba15e510, heapRelation=0x7ff21a1acef0,
checkUnique=UNIQUE_CHECK_NO, indexUnchanged=false, 
    indexInfo=0x5610ba15f9c8) at indexam.c:193
#13 0x00005610b8878e87 in ExecInsertIndexTuples
(resultRelInfo=0x5610ba1446e8, slot=0x5610ba15e4e0, 
    estate=0x5610ba142f90, update=false, noDupErr=false, specConflict=0x0,
arbiterIndexes=0x0) at execIndexing.c:416
#14 0x00005610b88c6a99 in ExecInsert (context=0x7ffe00cdb130,
resultRelInfo=0x5610ba1446e8, slot=0x5610ba15e4e0, 
    canSetTag=true, inserted_tuple=0x0, insert_destrel=0x0) at
nodeModifyTable.c:1120
#15 0x00005610b88cb096 in ExecModifyTable (pstate=0x5610ba1444d0) at
nodeModifyTable.c:3764
#16 0x00005610b88879a4 in ExecProcNodeFirst (node=0x5610ba1444d0) at
execProcnode.c:464
#17 0x00005610b887abd1 in ExecProcNode (node=0x5610ba1444d0) at
../../../src/include/executor/executor.h:259
#18 0x00005610b887dae4 in ExecutePlan (estate=0x5610ba142f90,
planstate=0x5610ba1444d0, use_parallel_mode=false, 
    operation=CMD_INSERT, sendTuples=false, numberTuples=0,
direction=ForwardScanDirection, dest=0x5610ba13cb08, 
    execute_once=true) at execMain.c:1636
#19 0x00005610b887b308 in standard_ExecutorRun (queryDesc=0x5610ba07a360,
direction=ForwardScanDirection, count=0, 
    execute_once=true) at execMain.c:363
#20 0x00005610b887b0f3 in ExecutorRun (queryDesc=0x5610ba07a360,
direction=ForwardScanDirection, count=0, 
    execute_once=true) at execMain.c:307
#21 0x00005610b8af4194 in ProcessQuery (plan=0x5610ba13ca18, 
    sourceText=0x5610ba057460 "INSERT INTO test__int SELECT array(SELECT x
FROM generate_series(1, 1001) x) FROM generate_series(1, 10);", params=0x0,
queryEnv=0x0, dest=0x5610ba13cb08, qc=0x7ffe00cdb580) at pquery.c:160
#22 0x00005610b8af5d8a in PortalRunMulti (portal=0x5610ba0e7070,
isTopLevel=true, setHoldSnapshot=false, 
    dest=0x5610ba13cb08, altdest=0x5610ba13cb08, qc=0x7ffe00cdb580) at
pquery.c:1277
#23 0x00005610b8af526d in PortalRun (portal=0x5610ba0e7070,
count=9223372036854775807, isTopLevel=true, run_once=true, 
    dest=0x5610ba13cb08, altdest=0x5610ba13cb08, qc=0x7ffe00cdb580) at
pquery.c:791
#24 0x00005610b8aee050 in exec_simple_query (
    query_string=0x5610ba057460 "INSERT INTO test__int SELECT array(SELECT x
FROM generate_series(1, 1001) x) FROM generate_series(1, 10);") at
postgres.c:1250
#25 0x00005610b8af2f74 in PostgresMain (dbname=0x5610ba085c48 "regression",
username=0x5610ba085c28 "law")
    at postgres.c:4593
#26 0x00005610b8a17e7c in BackendRun (port=0x5610ba07d360) at
postmaster.c:4511
#27 0x00005610b8a17703 in BackendStartup (port=0x5610ba07d360) at
postmaster.c:4239
#28 0x00005610b8a13678 in ServerLoop () at postmaster.c:1806
#29 0x00005610b8a12dd8 in PostmasterMain (argc=3, argv=0x5610ba052670) at
postmaster.c:1478
#30 0x00005610b89065f4 in main (argc=3, argv=0x5610ba052670) at main.c:202

IIUC, this check in g_int_decompress():
    lenin = ARRNELEMS(in);

    if (lenin < 2 * num_ranges)
    {                            /* not compressed value */
...
assumes that a larger number of elements is a sign of compressed value,
but
g_int_compress() can produce a non-compressed value with such number of
elements (emitting a notice just for information).

I'd also note that a variation with 2001 elements and the different order:
INSERT INTO test__int SELECT array(SELECT x FROM generate_series(1, 2001) x)
FROM generate_series(1, 10);
CREATE INDEX test_idx on test__int using gist (a gist__int_ops);

gives a confusing message:
ERROR:  index row size 8040 exceeds maximum 8152 for index "test_idx"

Reproduced on REL_11_STABLE .. master.

On Fri, Apr 7, 2023 at 4:21 AM PG Bug reporting form <noreply@postgresql.org> wrote:

NOTICE:  input array is too big (199 maximum allowed, 1001 current), use
gist__intbig_ops opclass instead

In g_int_compress,

 if (ARRNELEMS(r) >= 2 * num_ranges)
     elog(NOTICE, "input array is too big (%d maximum allowed, %d current), use gist__intbig_ops opclass instead",
          2 * num_ranges - 1, ARRNELEMS(r));

Wondering why elog with NOTICE rather than Error here.

The NOTICE appeared there in commit 08ee64ebf5 (from year 2005) in an
attempt to get rid of flags (ISLEAFKEY(in)) [1] and probably expressed
an intention to play gently (don't break compatibility?).

The patch proposed at [2] looks correct to me. Thank you, Ankit!
Though I would add a simple case to the intarray regression test and also
add errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED) to other two places in
_int_gist.c for the sake of consistency.

Please look at the patches attached.

[1] https://www.postgresql.org/message-id/43786274.3020200%40sigaev.ru
[2] https://www.postgresql.org/message-id/796b65c3-57b7-bddf-b0d5-a8afafb8b627%40gmail.com

On Wed, May 31, 2023 at 09:00:00AM +0300, Alexander Lakhin wrote:
> The NOTICE appeared there in commit 08ee64ebf5 (from year 2005) in an
> attempt to get rid of flags (ISLEAFKEY(in)) [1] and probably expressed
> an intention to play gently (don't break compatibility?).

Good question.  This commit or its related thread don't tell much.
FWIW, these are two things that don't make much sense to me:
1) to allow the compression to work with that many elements with its
code unable to handle that, while storing incorrect data.
2) the decompression fails entirely to cope with that.

> The patch proposed at [2] looks correct to me. Thank you, Ankit!
> Though I would add a simple case to the intarray regression test and also
> add errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED) to other two places in
> _int_gist.c for the sake of consistency.

The error is different in ~9.6 and 10~, where we would not get a crash
before that.  But We do get a lot of write past chunk end WARNINGs
instead.

Saying that, being able to corrupt the server at will with a gist
operator that can be installed is no good either, so your suggestion
of made of the two patches is OK by me.  Based on my read of the code,
this prevents the compression to store data it would not be able to
handle on decompression.  Now, I have to admit that I am not the most
verse with this area.

Likely that's something we'd better backpatch, to avoid people playing
with that too much in the back-branches.  Thoughts or objections?
--
Michael

On Wed, Jun 14, 2023 at 01:21:40PM +0900, Michael Paquier wrote:
> Likely that's something we'd better backpatch, to avoid people playing
> with that too much in the back-branches.  Thoughts or objections?

Well, we could perhaps revisit the choice of 08ee64e to remove the
LEAF flag from the data stored.  However, seeing the lack of
complaints for the past 18 years with users storing large arrays under
gist__int_ops, spending more time on this is not really appealing,
either.  Or we would have heard about that because the decompression
immediately breaks for this case.

I have spent some time on that today, and applied what has been
suggested to restrict the usage of this operator when inserting data
for a leaf page, so as the decompression does not get crazy when
retrieving the contents of the leaf pages.
--
Michael

Hi Michael,

15.06.2023 07:53, Michael Paquier wrote:
> I have spent some time on that today, and applied what has been
> suggested to restrict the usage of this operator when inserting data
> for a leaf page, so as the decompression does not get crazy when
> retrieving the contents of the leaf pages.

Thank you for paying attention to it!
I closed https://commitfest.postgresql.org/43/4280/ as committed.

The confusing message I noted previously:
ERROR:  index row size 8040 exceeds maximum 8152 for index "test_idx"
is not emitted now, although I think it still can be produced for some
other gist indexes. But that's a matter for another day, I suppose.

Best regards,
Alexander