public inbox for [email protected]
help / color / mirror / Atom feedFrom: Robert Haas <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: TupleDescAttr bounds checks
Date: Fri, 20 Mar 2026 11:58:08 -0400
Message-ID: <CA+TgmoacixUZVvi00hOjk_d9B4iYKswWP1gNqQ8Vfray-AcOCA@mail.gmail.com> (raw)
Hi,
Scrutiny of a recent test_plan_advice failure in the buildfarm
revealed a bug that had nothing to do with test_plan_advice or
pg_plan_advice; rather, it was a bug introduced by the virtual
generated columns feature, and specifically of that feature indexing
off of the beginning of a TupleDesc when whole-row attributes are
present. The first patch attached to this email fixes this issue, and
should be committed and back-patched to v18. I plan to do that soon
unless there are objections.
But that got me wondering why we don't have an assertion in
TupleDescAttr to catch this sort of thing, and it seems like that is
indeed something we can do, so patch #2 adds that and then cleans up
the resulting damage. By "damage" I mean correcting places where the
new Assert() either actually fails or could theoretically fail,
because we use TupleDescAttr() on a value that we don't know to be
within range. None of these seem to be actual bugs, because as the
commit message says, all TupleDescAttr() does is compute a pointer,
and we don't actually dereference that pointer in any of these code
paths until after we know that it's OK to do so. Nonetheless, these
all seem like good cleanups, so I do not see any of these changes as
arguments against adding the assertion. I propose to put this in
master.
Patch #3 adds a test case that would have caught the bug fixed by
patch #1 if we had already had the asserts added by patch #2. To my
surprise, we seem to have zero existing test coverage of creating an
index on a whole-row expression, so I think this is worth adding
mostly for that reason. One could also argue that it's worth adding as
a follow-up to #1 and #2, but we're unlikely to reintroduce that
specific bug. We might, however, add other bugs that this would also
catch.
Comments?
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] v1-0001-Prevent-spurious-indexes-on-virtual-generated-col.patch (2.2K, 2-v1-0001-Prevent-spurious-indexes-on-virtual-generated-col.patch)
download | inline diff:
From 36829f53acbbadb0e24254952a6d6ec036f41639 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Fri, 20 Mar 2026 11:09:44 -0400
Subject: [PATCH v1 1/3] Prevent spurious "indexes on virtual generated columns
are not supported".
Both of the checks in DefineIndex() that can produce this error
message have a guard against negative attribute numbers, but lack a
guard to ensure that attno is non-zero. As a result, we can index
off the beginning of the TupleDesc and read a garbage byte for
attgenerated. If that byte happens to be 'v', we'll incorrectly
produce the error mentioned above.
The first call site is easy to hit: any attempt to create an
expression index does so. The second one is not currently hit in
the regression tests, but can be hit by something like
CREATE INDEX ON some_table ((some_function(some_table))).
Found by study of a test_plan_advice failure on buildfarm member
skink, though this issue has nothing to do with test_plan_advice
and seems to have only been revealed by happenstance.
Backpatch-through: 18
---
src/backend/commands/indexcmds.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b89c6855364..dd593ccbc1c 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1123,7 +1123,8 @@ DefineIndex(ParseState *pstate,
errmsg("index creation on system columns is not supported")));
- if (TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+ if (attno > 0 &&
+ TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
stmt->primary ?
@@ -1164,7 +1165,8 @@ DefineIndex(ParseState *pstate,
{
AttrNumber attno = j + FirstLowInvalidHeapAttributeNumber;
- if (TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+ if (attno > 0 &&
+ TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
stmt->isconstraint ?
--
2.51.0
[application/octet-stream] v1-0003-Add-a-test-for-creating-an-index-on-a-whole-row-e.patch (2.6K, 3-v1-0003-Add-a-test-for-creating-an-index-on-a-whole-row-e.patch)
download | inline diff:
From 04e0aee0ab7cc6b05afed3fa8479d168225918bb Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Fri, 20 Mar 2026 11:36:11 -0400
Subject: [PATCH v1 3/3] Add a test for creating an index on a whole-row
expression.
Surprisingly, we have no existing test for this. Had this test
been present before commit 0000000000000000000000000000000000000000,
the Assert added in commit 0000000000000000000000000000000000000000
would have caught the bug.
!!! Update this with the final commit hashes.
---
src/test/regress/expected/indexing.out | 11 +++++++++++
src/test/regress/sql/indexing.sql | 12 ++++++++++++
2 files changed, 23 insertions(+)
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index dc629928c8f..f50868ca6a6 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -1669,3 +1669,14 @@ reindex index test_pg_index_toast_index;
drop index test_pg_index_toast_index;
drop function test_pg_index_toast_func;
drop table test_pg_index_toast_table;
+-- test creation of an index involving a whole-row expression
+create table test_pg_wholerow_index (a int, b text, c numeric);
+create or replace function row_image(test_pg_wholerow_index)
+ returns test_pg_wholerow_index as $$select $1$$ language sql immutable;
+insert into test_pg_wholerow_index values (1, 'multiplication', 1.0);
+create index row_image_index
+ on test_pg_wholerow_index ((row_image(test_pg_wholerow_index)));
+insert into test_pg_wholerow_index values (2, 'addition', 0);
+drop index row_image_index;
+drop function row_image(test_pg_wholerow_index);
+drop table test_pg_wholerow_index;
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index b5cb01c2d70..129130d04d4 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -934,3 +934,15 @@ reindex index test_pg_index_toast_index;
drop index test_pg_index_toast_index;
drop function test_pg_index_toast_func;
drop table test_pg_index_toast_table;
+
+-- test creation of an index involving a whole-row expression
+create table test_pg_wholerow_index (a int, b text, c numeric);
+create or replace function row_image(test_pg_wholerow_index)
+ returns test_pg_wholerow_index as $$select $1$$ language sql immutable;
+insert into test_pg_wholerow_index values (1, 'multiplication', 1.0);
+create index row_image_index
+ on test_pg_wholerow_index ((row_image(test_pg_wholerow_index)));
+insert into test_pg_wholerow_index values (2, 'addition', 0);
+drop index row_image_index;
+drop function row_image(test_pg_wholerow_index);
+drop table test_pg_wholerow_index;
--
2.51.0
[application/octet-stream] v1-0002-Bounds-check-access-to-TupleDescAttr-with-an-Asse.patch (5.9K, 4-v1-0002-Bounds-check-access-to-TupleDescAttr-with-an-Asse.patch)
download | inline diff:
From bf15cdc8dc96af8c56f7a4e3ba0c49acf50f480f Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Fri, 20 Mar 2026 11:27:05 -0400
Subject: [PATCH v1 2/3] Bounds-check access to TupleDescAttr with an Assert.
The second argument to TupleDescAttr should always be at least zero
and less than natts; otherwise, we index outside of the attribute
array. Assert that this is the case.
Various violations, or possible violations, of this rule that are
currently in the tree are actually harmless, because while
we do call TupleDescAttr() before verifying that the argument is
within range, we don't actually dereference it unless the argument
was within range all along. Nonetheless, the Assert means we
should be more careful, so tidy up accordingly.
---
src/backend/access/common/tupdesc.c | 27 +++++++++++++++------------
src/include/access/tupdesc.h | 2 ++
src/pl/plperl/plperl.c | 7 +++++--
src/pl/plpgsql/src/pl_exec.c | 6 ++++--
4 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index d771a265b34..196472c05d0 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -246,10 +246,11 @@ CreateTupleDescCopy(TupleDesc tupdesc)
desc = CreateTemplateTupleDesc(tupdesc->natts);
- /* Flat-copy the attribute array */
- memcpy(TupleDescAttr(desc, 0),
- TupleDescAttr(tupdesc, 0),
- desc->natts * sizeof(FormData_pg_attribute));
+ /* Flat-copy the attribute array (unless there are no attributes) */
+ if (desc->natts > 0)
+ memcpy(TupleDescAttr(desc, 0),
+ TupleDescAttr(tupdesc, 0),
+ desc->natts * sizeof(FormData_pg_attribute));
/*
* Since we're not copying constraints and defaults, clear fields
@@ -294,10 +295,11 @@ CreateTupleDescTruncatedCopy(TupleDesc tupdesc, int natts)
desc = CreateTemplateTupleDesc(natts);
- /* Flat-copy the attribute array */
- memcpy(TupleDescAttr(desc, 0),
- TupleDescAttr(tupdesc, 0),
- desc->natts * sizeof(FormData_pg_attribute));
+ /* Flat-copy the attribute array (unless there are no attributes) */
+ if (desc->natts > 0)
+ memcpy(TupleDescAttr(desc, 0),
+ TupleDescAttr(tupdesc, 0),
+ desc->natts * sizeof(FormData_pg_attribute));
/*
* Since we're not copying constraints and defaults, clear fields
@@ -339,10 +341,11 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
desc = CreateTemplateTupleDesc(tupdesc->natts);
- /* Flat-copy the attribute array */
- memcpy(TupleDescAttr(desc, 0),
- TupleDescAttr(tupdesc, 0),
- desc->natts * sizeof(FormData_pg_attribute));
+ /* Flat-copy the attribute array (unless there are no attributes) */
+ if (desc->natts > 0)
+ memcpy(TupleDescAttr(desc, 0),
+ TupleDescAttr(tupdesc, 0),
+ desc->natts * sizeof(FormData_pg_attribute));
for (i = 0; i < desc->natts; i++)
{
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index 62ef6b38497..d26287271e9 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -179,6 +179,8 @@ TupleDescAttr(TupleDesc tupdesc, int i)
{
FormData_pg_attribute *attrs = TupleDescAttrAddress(tupdesc);
+ Assert(i >= 0 && i < tupdesc->natts);
+
return &attrs[i];
}
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index c5f11b874c7..06ebffa111c 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -1093,7 +1093,7 @@ plperl_build_tuple_result(HV *perlhash, TupleDesc td)
SV *val = HeVAL(he);
char *key = hek2cstr(he);
int attn = SPI_fnumber(td, key);
- Form_pg_attribute attr = TupleDescAttr(td, attn - 1);
+ Form_pg_attribute attr;
if (attn == SPI_ERROR_NOATTRIBUTE)
ereport(ERROR,
@@ -1106,6 +1106,7 @@ plperl_build_tuple_result(HV *perlhash, TupleDesc td)
errmsg("cannot set system attribute \"%s\"",
key)));
+ attr = TupleDescAttr(td, attn - 1);
values[attn - 1] = plperl_sv_to_datum(val,
attr->atttypid,
attr->atttypmod,
@@ -1799,7 +1800,7 @@ plperl_modify_tuple(HV *hvTD, TriggerData *tdata, HeapTuple otup)
char *key = hek2cstr(he);
SV *val = HeVAL(he);
int attn = SPI_fnumber(tupdesc, key);
- Form_pg_attribute attr = TupleDescAttr(tupdesc, attn - 1);
+ Form_pg_attribute attr;
if (attn == SPI_ERROR_NOATTRIBUTE)
ereport(ERROR,
@@ -1811,6 +1812,8 @@ plperl_modify_tuple(HV *hvTD, TriggerData *tdata, HeapTuple otup)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot set system attribute \"%s\"",
key)));
+
+ attr = TupleDescAttr(tupdesc, attn - 1);
if (attr->attgenerated)
ereport(ERROR,
(errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 84552e32c87..a3869072093 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3369,7 +3369,7 @@ exec_stmt_return_next(PLpgSQL_execstate *estate,
PLpgSQL_var *var = (PLpgSQL_var *) retvar;
Datum retval = var->value;
bool isNull = var->isnull;
- Form_pg_attribute attr = TupleDescAttr(tupdesc, 0);
+ Form_pg_attribute attr;
if (natts != 1)
ereport(ERROR,
@@ -3382,6 +3382,7 @@ exec_stmt_return_next(PLpgSQL_execstate *estate,
var->datatype->typlen);
/* coerce type if needed */
+ attr = TupleDescAttr(tupdesc, 0);
retval = exec_cast_value(estate,
retval,
&isNull,
@@ -3500,7 +3501,7 @@ exec_stmt_return_next(PLpgSQL_execstate *estate,
}
else
{
- Form_pg_attribute attr = TupleDescAttr(tupdesc, 0);
+ Form_pg_attribute attr;
/* Simple scalar result */
if (natts != 1)
@@ -3509,6 +3510,7 @@ exec_stmt_return_next(PLpgSQL_execstate *estate,
errmsg("wrong result type supplied in RETURN NEXT")));
/* coerce type if needed */
+ attr = TupleDescAttr(tupdesc, 0);
retval = exec_cast_value(estate,
retval,
&isNull,
--
2.51.0
view thread (10+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: TupleDescAttr bounds checks
In-Reply-To: <CA+TgmoacixUZVvi00hOjk_d9B4iYKswWP1gNqQ8Vfray-AcOCA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox