Skip to content

Commit 1fa2417

Browse files
buriedpotburiedpot
committed
fix: avoid using uninitialized memory
During a RUM index scan, if a concurrent VACUUM completes and removes all items from a posting tree leaf page or a posting list, entry->nlist can become zero. In this case, entry->curItem may point to uninitialized memory, leading to a crash. The commit fixes this bug.
1 parent f557ab1 commit 1fa2417

File tree

4 files changed

+299
-8
lines changed

4 files changed

+299
-8
lines changed

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ REGRESS = security rum rum_validate rum_hash ruminv timestamp \
2626
int2 int4 int8 float4 float8 money oid \
2727
time timetz date interval \
2828
macaddr inet cidr text varchar char bytea bit varbit \
29-
numeric rum_weight expr array
29+
numeric rum_weight expr array rum_vacuum
3030

3131
TAP_TESTS = 1
3232

‎expected/rum_vacuum.out‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
-- Test RUM index scan correctness after concurrent VACUUM removes all
2+
-- posting tree entry items.
3+
SET enable_seqscan TO off;
4+
SET enable_indexscan TO off;
5+
SET enable_bitmapscan TO on;
6+
CREATE TABLE test_rum_vacuum (id int, body tsvector);
7+
ALTER TABLE test_rum_vacuum SET (autovacuum_enabled = false);
8+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great ann') FROM generate_series(1, 6) i;
9+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great john') FROM generate_series(7, 10000) i;
10+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great james') FROM generate_series(10001, 10003) i;
11+
CREATE INDEX ON test_rum_vacuum USING rum (body rum_tsvector_ops);
12+
DELETE FROM test_rum_vacuum WHERE body @@ 'ann'::tsquery AND id <= 5;
13+
DELETE FROM test_rum_vacuum WHERE body @@ 'john'::tsquery AND id <= 9999;
14+
-- test normal result
15+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('ann');
16+
id | body
17+
----+-------------------
18+
6 | 'ann':2 'great':1
19+
(1 row)
20+
21+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
22+
id | body
23+
-------+--------------------
24+
10000 | 'great':1 'john':2
25+
(1 row)
26+
27+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('ann') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('ann')) AS sub ORDER BY distance ASC, id ASC;
28+
id | body
29+
-------+--------------------
30+
6 | 'ann':2 'great':1
31+
10000 | 'great':1 'john':2
32+
10001 | 'great':1 'jame':2
33+
10002 | 'great':1 'jame':2
34+
10003 | 'great':1 'jame':2
35+
(5 rows)
36+
37+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('john') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('john')) AS sub ORDER BY distance ASC, id ASC;
38+
id | body
39+
-------+--------------------
40+
10000 | 'great':1 'john':2
41+
6 | 'ann':2 'great':1
42+
10001 | 'great':1 'jame':2
43+
10002 | 'great':1 'jame':2
44+
10003 | 'great':1 'jame':2
45+
(5 rows)
46+
47+
VACUUM test_rum_vacuum;
48+
-- this shouldn't cause a core dump
49+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('ann');
50+
id | body
51+
----+-------------------
52+
6 | 'ann':2 'great':1
53+
(1 row)
54+
55+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
56+
id | body
57+
-------+--------------------
58+
10000 | 'great':1 'john':2
59+
(1 row)
60+
61+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('ann') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('ann')) AS sub ORDER BY distance ASC, id ASC;
62+
id | body
63+
-------+--------------------
64+
6 | 'ann':2 'great':1
65+
10000 | 'great':1 'john':2
66+
10001 | 'great':1 'jame':2
67+
10002 | 'great':1 'jame':2
68+
10003 | 'great':1 'jame':2
69+
(5 rows)
70+
71+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('john') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('john')) AS sub ORDER BY distance ASC, id ASC;
72+
id | body
73+
-------+--------------------
74+
10000 | 'great':1 'john':2
75+
6 | 'ann':2 'great':1
76+
10001 | 'great':1 'jame':2
77+
10002 | 'great':1 'jame':2
78+
10003 | 'great':1 'jame':2
79+
(5 rows)
80+
81+
-- test that data can still be found after reinsertion
82+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great john') FROM generate_series(10004, 20000) i;
83+
SELECT count(*) FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
84+
count
85+
-------
86+
9998
87+
(1 row)
88+
89+
DELETE FROM test_rum_vacuum WHERE body @@ 'john'::tsquery AND id <= 19999;
90+
VACUUM test_rum_vacuum;
91+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('ann');
92+
id | body
93+
----+-------------------
94+
6 | 'ann':2 'great':1
95+
(1 row)
96+
97+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
98+
id | body
99+
-------+--------------------
100+
20000 | 'great':1 'john':2
101+
(1 row)
102+
103+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('ann') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('ann')) AS sub ORDER BY distance ASC, id ASC;
104+
id | body
105+
-------+--------------------
106+
6 | 'ann':2 'great':1
107+
10001 | 'great':1 'jame':2
108+
10002 | 'great':1 'jame':2
109+
10003 | 'great':1 'jame':2
110+
20000 | 'great':1 'john':2
111+
(5 rows)
112+
113+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('john') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('john')) AS sub ORDER BY distance ASC, id ASC;
114+
id | body
115+
-------+--------------------
116+
20000 | 'great':1 'john':2
117+
6 | 'ann':2 'great':1
118+
10001 | 'great':1 'jame':2
119+
10002 | 'great':1 'jame':2
120+
10003 | 'great':1 'jame':2
121+
(5 rows)
122+
123+
-- test if do while loop works when an entry has no non-empty posting tree pages
124+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great john') FROM generate_series(7, 10000) i;
125+
DELETE FROM test_rum_vacuum WHERE body @@ 'john'::tsquery;
126+
VACUUM test_rum_vacuum;
127+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('ann');
128+
id | body
129+
----+-------------------
130+
6 | 'ann':2 'great':1
131+
(1 row)
132+
133+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
134+
id | body
135+
----+------
136+
(0 rows)
137+
138+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('ann') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('ann')) AS sub ORDER BY distance ASC, id ASC;
139+
id | body
140+
-------+--------------------
141+
6 | 'ann':2 'great':1
142+
10001 | 'great':1 'jame':2
143+
10002 | 'great':1 'jame':2
144+
10003 | 'great':1 'jame':2
145+
(4 rows)
146+
147+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('john') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('john')) AS sub ORDER BY distance ASC, id ASC;
148+
id | body
149+
-------+--------------------
150+
6 | 'ann':2 'great':1
151+
10001 | 'great':1 'jame':2
152+
10002 | 'great':1 'jame':2
153+
10003 | 'great':1 'jame':2
154+
(4 rows)

‎sql/rum_vacuum.sql‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
-- Test RUM index scan correctness after concurrent VACUUM removes all
2+
-- posting tree entry items.
3+
4+
SET enable_seqscan TO off;
5+
SET enable_indexscan TO off;
6+
SET enable_bitmapscan TO on;
7+
8+
CREATE TABLE test_rum_vacuum (id int, body tsvector);
9+
ALTER TABLE test_rum_vacuum SET (autovacuum_enabled = false);
10+
11+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great ann') FROM generate_series(1, 6) i;
12+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great john') FROM generate_series(7, 10000) i;
13+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great james') FROM generate_series(10001, 10003) i;
14+
15+
CREATE INDEX ON test_rum_vacuum USING rum (body rum_tsvector_ops);
16+
17+
DELETE FROM test_rum_vacuum WHERE body @@ 'ann'::tsquery AND id <= 5;
18+
DELETE FROM test_rum_vacuum WHERE body @@ 'john'::tsquery AND id <= 9999;
19+
20+
-- test normal result
21+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('ann');
22+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
23+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('ann') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('ann')) AS sub ORDER BY distance ASC, id ASC;
24+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('john') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('john')) AS sub ORDER BY distance ASC, id ASC;
25+
26+
VACUUM test_rum_vacuum;
27+
28+
-- this shouldn't cause a core dump
29+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('ann');
30+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
31+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('ann') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('ann')) AS sub ORDER BY distance ASC, id ASC;
32+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('john') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('john')) AS sub ORDER BY distance ASC, id ASC;
33+
34+
-- test that data can still be found after reinsertion
35+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great john') FROM generate_series(10004, 20000) i;
36+
SELECT count(*) FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
37+
DELETE FROM test_rum_vacuum WHERE body @@ 'john'::tsquery AND id <= 19999;
38+
39+
VACUUM test_rum_vacuum;
40+
41+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('ann');
42+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
43+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('ann') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('ann')) AS sub ORDER BY distance ASC, id ASC;
44+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('john') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('john')) AS sub ORDER BY distance ASC, id ASC;
45+
46+
-- test if do while loop works when an entry has no non-empty posting tree pages
47+
INSERT INTO test_rum_vacuum SELECT i, to_tsvector('great john') FROM generate_series(7, 10000) i;
48+
DELETE FROM test_rum_vacuum WHERE body @@ 'john'::tsquery;
49+
VACUUM test_rum_vacuum;
50+
51+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('ann');
52+
SELECT * FROM test_rum_vacuum WHERE body @@ to_tsquery('john');
53+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('ann') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('ann')) AS sub ORDER BY distance ASC, id ASC;
54+
SELECT id, body FROM (SELECT id, body, body <=> to_tsquery('john') AS distance FROM test_rum_vacuum ORDER BY body <=> to_tsquery('john')) AS sub ORDER BY distance ASC, id ASC;

‎src/rumget.c‎

Lines changed: 90 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -684,9 +684,23 @@ startScanEntry(RumState * rumstate, RumScanEntry entry, Snapshot snapshot)
684684
}
685685

686686
LockBuffer(entry->buffer, RUM_UNLOCK);
687-
entry->isFinished = setListPositionScanEntry(rumstate, entry);
688-
if (!entry->isFinished)
689-
entry->curItem = entry->list[entry->offset];
687+
688+
/*
689+
* If the current page is empty (nlist == 0), we cannot assume the
690+
* scan is complete, as subsequent pages may exist. Therefore, we
691+
* set isFinished = false and leave entry->nlist = 0 and
692+
* entry->offset = 0 to ensure that entryGetItem advances to the
693+
* next page on the next call. Otherwise, initialize curItem to
694+
* the first valid item.
695+
*/
696+
if (entry->nlist == 0)
697+
entry->isFinished = false;
698+
else
699+
{
700+
entry->isFinished = setListPositionScanEntry(rumstate, entry);
701+
if (!entry->isFinished)
702+
entry->curItem = entry->list[entry->offset];
703+
}
690704
}
691705
else if (RumGetNPosting(itup) > 0)
692706
{
@@ -699,6 +713,16 @@ startScanEntry(RumState * rumstate, RumScanEntry entry, Snapshot snapshot)
699713
if (!entry->isFinished)
700714
entry->curItem = entry->list[entry->offset];
701715
}
716+
/*
717+
* Else, the posting list for this entry has been entirely vacuumed
718+
* away (nlist == 0 after setListPositionScanEntry). We cannot assume
719+
* the scan is complete, as subsequent pages may exist. Therefore, we
720+
* set isFinished = false and leave entry->nlist = 0 and entry->offset
721+
* = 0 to ensure that entryGetItem advances to the next page on the
722+
* next call.
723+
*/
724+
else
725+
entry->isFinished = false;
702726

703727
if (entry->queryCategory == RUM_CAT_EMPTY_QUERY &&
704728
entry->scanWithAddInfo)
@@ -1011,6 +1035,13 @@ entryGetNextItem(RumState * rumstate, RumScanEntry entry, Snapshot snapshot)
10111035

10121036
LockBuffer(entry->buffer, RUM_UNLOCK);
10131037

1038+
/*
1039+
* No valid item if VACUUM removed all items concurrently. Go on
1040+
* next page.
1041+
*/
1042+
if (entry->nlist == 0)
1043+
break;
1044+
10141045
if (entry->offset < 0)
10151046
{
10161047
if (ScanDirectionIsForward(entry->scanDirection) &&
@@ -1044,6 +1075,7 @@ entryGetNextItemList(RumState * rumstate, RumScanEntry entry, Snapshot snapshot)
10441075
RumItemSetMin(&entry->curItem);
10451076
entry->offset = InvalidOffsetNumber;
10461077
entry->list = NULL;
1078+
entry->nlist = 0;
10471079
if (entry->gdi)
10481080
{
10491081
freeRumBtreeStack(entry->gdi->stack);
@@ -1151,6 +1183,18 @@ entryGetNextItemList(RumState * rumstate, RumScanEntry entry, Snapshot snapshot)
11511183

11521184
LockBuffer(entry->buffer, RUM_UNLOCK);
11531185
entry->isFinished = false;
1186+
1187+
/*
1188+
* Posting tree's first leaf page is empty due to concurrent VACUUM.
1189+
* Advance through empty pages until we find one with items or exhaust
1190+
* the tree. entryGetItem does not re-invoke entryGetNextItem after we
1191+
* return, so we must do it here to ensure curItem is valid on return.
1192+
*/
1193+
if (entry->nlist == 0)
1194+
{
1195+
entryGetNextItem(rumstate, entry, snapshot);
1196+
goto entry_done;
1197+
}
11541198
}
11551199
else if (RumGetNPosting(itup) > 0)
11561200
{
@@ -1161,12 +1205,21 @@ entryGetNextItemList(RumState * rumstate, RumScanEntry entry, Snapshot snapshot)
11611205
rumReadTuple(rumstate, entry->attnum, itup, entry->list, true);
11621206
entry->isFinished = setListPositionScanEntry(rumstate, entry);
11631207
}
1208+
/* Posting list has been vacuumed. Go to the next entry. */
1209+
else
1210+
{
1211+
ItemPointerSetInvalid(&entry->curItem.iptr);
1212+
entry->isFinished = true;
1213+
goto entry_done;
1214+
}
11641215

11651216
Assert(entry->nlist > 0 && entry->list);
11661217

11671218
entry->curItem = entry->list[entry->offset];
11681219
entry->offset += entry->scanDirection;
11691220

1221+
entry_done:
1222+
11701223
SCAN_ENTRY_GET_KEY(entry, rumstate, itup);
11711224

11721225
/*
@@ -1340,8 +1393,24 @@ entryGetItem(RumState * rumstate, RumScanEntry entry, bool *nextEntryList, Snaps
13401393
}
13411394
else if (entry->stack)
13421395
{
1343-
entry->offset++;
1344-
if (entryGetNextItemList(rumstate, entry, snapshot) && nextEntryList)
1396+
/*
1397+
* We are responsible for ensuring that we keep advancing through
1398+
* ItemLists until we find one that contains at least one valid
1399+
* item. This is necessary because concurrent VACUUM may have
1400+
* removed all items from a page, leaving an empty ItemList. In
1401+
* such cases, we must continue to the next ItemList.
1402+
*/
1403+
bool success;
1404+
1405+
Assert(!entry->isFinished);
1406+
1407+
do
1408+
{
1409+
entry->isFinished = false;
1410+
success = entryGetNextItemList(rumstate, entry, snapshot);
1411+
} while (success && entry->nlist == 0);
1412+
1413+
if (success && nextEntryList)
13451414
*nextEntryList = true;
13461415
}
13471416
else
@@ -1361,8 +1430,22 @@ entryGetItem(RumState * rumstate, RumScanEntry entry, bool *nextEntryList, Snaps
13611430
dropItem(entry));
13621431
if (entry->stack && entry->isFinished)
13631432
{
1364-
entry->isFinished = false;
1365-
if (entryGetNextItemList(rumstate, entry, snapshot) && nextEntryList)
1433+
/*
1434+
* We are responsible for ensuring that we keep advancing through
1435+
* ItemLists until we find one that contains at least one valid
1436+
* item. This is necessary because concurrent VACUUM may have
1437+
* removed all items from a page, leaving an empty ItemList. In
1438+
* such cases, we must continue to the next ItemList.
1439+
*/
1440+
bool success;
1441+
1442+
do
1443+
{
1444+
entry->isFinished = false;
1445+
success = entryGetNextItemList(rumstate, entry, snapshot);
1446+
} while (success && entry->nlist == 0);
1447+
1448+
if (success && nextEntryList)
13661449
*nextEntryList = true;
13671450
}
13681451
}

0 commit comments

Comments
 (0)