Thread: BUG #17877: Referencing a system column in a foreign key leads to incorrect memory access
BUG #17877: Referencing a system column in a foreign key leads to incorrect memory access
From
PG Bug reporting form
Date:
The following bug has been logged on the website:
Bug reference: 17877
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 15.2
Operating system: Ubuntu 22.04
Description:
The following query:
CREATE TABLE pt(tid oid, id int, PRIMARY KEY(tid, id));
CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES pt);
produces a valgrind-detected error:
==00:00:00:04.173 3911860== Invalid read of size 1
==00:00:00:04.173 3911860== at 0x3B032C: ATAddForeignKeyConstraint
(tablecmds.c:9242)
==00:00:00:04.173 3911860== by 0x3B2132: ATExecAddConstraint
(tablecmds.c:8875)
==00:00:00:04.173 3911860== by 0x3B8D73: ATExecCmd (tablecmds.c:5031)
==00:00:00:04.173 3911860== by 0x3B93F5: ATRewriteCatalogs
(tablecmds.c:4874)
==00:00:00:04.173 3911860== by 0x3B9613: ATController
(tablecmds.c:4451)
==00:00:00:04.173 3911860== by 0x3B969D: AlterTable (tablecmds.c:4097)
==00:00:00:04.173 3911860== by 0x596AD6: ProcessUtilitySlow
(utility.c:1325)
==00:00:00:04.173 3911860== by 0x596584: standard_ProcessUtility
(utility.c:1074)
==00:00:00:04.173 3911860== by 0x59666D: ProcessUtility (utility.c:530)
==00:00:00:04.173 3911860== by 0x59696B: ProcessUtilitySlow
(utility.c:1252)
==00:00:00:04.173 3911860== by 0x596584: standard_ProcessUtility
(utility.c:1074)
==00:00:00:04.173 3911860== by 0x59666D: ProcessUtility (utility.c:530)
==00:00:00:04.173 3911860== Address 0x4bf512dc is 113,516 bytes inside a
recently re-allocated block of size 524,288 alloc'd
...
2023-03-29 16:00:29.068 MSK|||64243669.3bb06e|LOG: server process (PID
3911860) exited with exit code 1
2023-03-29 16:00:29.068 MSK|||64243669.3bb06e|DETAIL: Failed process was
running: CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES
pt);
Here in ATAddForeignKeyConstraint():
/*
* Check some things for generated columns.
*/
for (i = 0; i < numfks; i++)
{
char attgenerated = TupleDescAttr(RelationGetDescr(rel), fkattnum[i] -
1)->attgenerated;
if (attgenerated)
...
we have fkattnum[i] = -6 for the attribute "tableoid", but TupleDescAttr()
can't handle system attributes.
I've looked at fc22b6623 and found no other TupleDescAttr() calls where
attnum can be negative.
Though maybe it is a nonsense to use tableoid in a such way.
But if not, someone can try to move on without valgrind:
CREATE TABLE pt(tid oid, id int, PRIMARY KEY(tid, id));
CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES pt);
INSERT INTO pt SELECT oid, 1 FROM pg_class WHERE relname = 'ft';
INSERT INTO ft VALUES(1);
and get:
...
#5 0x0000557485037238 in ExceptionalCondition (conditionName=0x557485245f18
"attnum > 0",
errorType=0x557485245f0c "BadArgument",
fileName=0x557485245ee0 "../../../../src/include/executor/tuptable.h",
lineNumber=369) at assert.c:69
#6 0x0000557484f93846 in slot_attisnull (slot=0x557485e64420, attnum=-6)
at ../../../../src/include/executor/tuptable.h:369
#7 0x0000557484f9913c in ri_NullCheck (tupDesc=0x7fa297361aa8,
slot=0x557485e64420,
riinfo=0x557485e7ab80, rel_is_pk=false) at ri_triggers.c:2648
#8 0x0000557484f93bad in RI_FKey_check (trigdata=0x7fffc6ac8980) at
ri_triggers.c:280
#9 0x0000557484f940db in RI_FKey_check_ins (fcinfo=0x7fffc6ac87c0) at
ri_triggers.c:434
#10 0x0000557484bb9baf in ExecCallTriggerFunc (trigdata=0x7fffc6ac8980,
tgindx=0, finfo=0x557485e63878,
instr=0x0, per_tuple_context=0x557485e70f60) at trigger.c:2435
#11 0x0000557484bbd653 in AfterTriggerExecute (estate=0x557485e62ff0,
event=0x557485e6f090,
relInfo=0x557485e63480, src_relInfo=0x557485e63480,
dst_relInfo=0x557485e63480,
trigdesc=0x557485e63698, finfo=0x557485e63878, instr=0x0,
per_tuple_context=0x557485e70f60,
trig_tuple_slot1=0x0, trig_tuple_slot2=0x0) at trigger.c:4546
#12 0x0000557484bbdc68 in afterTriggerInvokeEvents (events=0x557485d8bdb0,
firing_id=1,
estate=0x557485e62ff0, delete_ok=false) at trigger.c:4785
#13 0x0000557484bbe5b5 in AfterTriggerEndQuery (estate=0x557485e62ff0) at
trigger.c:5140
#14 0x0000557484bf696b in standard_ExecutorFinish (queryDesc=0x557485e53c60)
at execMain.c:438
#15 0x0000557484bf6821 in ExecutorFinish (queryDesc=0x557485e53c60) at
execMain.c:406
#16 0x0000557484e6f649 in ProcessQuery (plan=0x557485e48798,
sourceText=0x557485d68870 "INSERT INTO ft VALUES(1);", params=0x0,
queryEnv=0x0, dest=0x557485e48888,
qc=0x7fffc6ac8e10) at pquery.c:193
Re: BUG #17877: Referencing a system column in a foreign key leads to incorrect memory access
From
Tom Lane
Date:
PG Bug reporting form <noreply@postgresql.org> writes:
> The following query:
> CREATE TABLE pt(tid oid, id int, PRIMARY KEY(tid, id));
> CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES pt);
> produces a valgrind-detected error:
We should probably just disallow system columns as foreign keys.
There was a legitimate use-case for that with OID columns, but
no more. I can't see a really good reason to use tableoid as a
foreign key, and none of the other system columns are stable
enough for this to be sane at all. So it's hard to summon
interest in trying to remove bugs of this sort.
regards, tom lane
Re: BUG #17877: Referencing a system column in a foreign key leads to incorrect memory access
From
Alvaro Herrera
Date:
On 2023-Mar-29, Tom Lane wrote: > PG Bug reporting form <noreply@postgresql.org> writes: > > The following query: > > CREATE TABLE pt(tid oid, id int, PRIMARY KEY(tid, id)); > > CREATE TABLE ft(id int, FOREIGN KEY (tableoid, id) REFERENCES pt); > > produces a valgrind-detected error: > > We should probably just disallow system columns as foreign keys. > There was a legitimate use-case for that with OID columns, but > no more. +1 -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Re: BUG #17877: Referencing a system column in a foreign key leads to incorrect memory access
From
Tom Lane
Date:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Mar-29, Tom Lane wrote:
>> We should probably just disallow system columns as foreign keys.
>> There was a legitimate use-case for that with OID columns, but
>> no more.
> +1
Seems pretty easy, as attached. The possibly harder question is whether
to back-patch this, or to try to fix the problems Alexander identified
in the back branches.
Some investigation determined that system columns in foreign keys
appear to actually work up through v11, which perhaps coincidentally
is the last version that allowed the OID system column. v12 and up
have the slot_attnum problem that Alexander showed. The lack of
complaints about that suggests that nobody is trying to use any other
system columns as FKs in production. Nonetheless, it's a bit worrisome
to remove a feature-that-used-to-work in stable branches.
On balance though, I'd rather block this than promise to make it work
in the back branches. I propose applying the attached back to v12,
and leaving v11 alone.
regards, tom lane
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3147dddf28..7808241d3f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11271,6 +11271,11 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName,
* transformColumnNameList - transform list of column names
*
* Lookup each name and return its attnum and, optionally, type OID
+ *
+ * Note: the name of this function suggests that it's general-purpose,
+ * but actually it's only used to look up names appearing in foreign-key
+ * clauses. The error messages would need work to use it in other cases,
+ * and perhaps the validity checks as well.
*/
static int
transformColumnNameList(Oid relId, List *colList,
@@ -11284,6 +11289,7 @@ transformColumnNameList(Oid relId, List *colList,
{
char *attname = strVal(lfirst(l));
HeapTuple atttuple;
+ Form_pg_attribute attform;
atttuple = SearchSysCacheAttName(relId, attname);
if (!HeapTupleIsValid(atttuple))
@@ -11291,14 +11297,19 @@ transformColumnNameList(Oid relId, List *colList,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" referenced in foreign key constraint does not exist",
attname)));
+ attform = (Form_pg_attribute) GETSTRUCT(atttuple);
+ if (attform->attnum < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("system columns cannot be used in foreign keys")));
if (attnum >= INDEX_MAX_KEYS)
ereport(ERROR,
(errcode(ERRCODE_TOO_MANY_COLUMNS),
errmsg("cannot have more than %d keys in a foreign key",
INDEX_MAX_KEYS)));
- attnums[attnum] = ((Form_pg_attribute) GETSTRUCT(atttuple))->attnum;
+ attnums[attnum] = attform->attnum;
if (atttypids != NULL)
- atttypids[attnum] = ((Form_pg_attribute) GETSTRUCT(atttuple))->atttypid;
+ atttypids[attnum] = attform->atttypid;
ReleaseSysCache(atttuple);
attnum++;
}
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 2f9c083539..55f7158c1a 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -795,15 +795,16 @@ SELECT * FROM FKTABLE ORDER BY id;
DROP TABLE FKTABLE;
DROP TABLE PKTABLE;
-CREATE TABLE PKTABLE (ptest1 int PRIMARY KEY);
+-- Test some invalid FK definitions
+CREATE TABLE PKTABLE (ptest1 int PRIMARY KEY, someoid oid);
CREATE TABLE FKTABLE_FAIL1 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (ftest2) REFERENCES PKTABLE);
ERROR: column "ftest2" referenced in foreign key constraint does not exist
CREATE TABLE FKTABLE_FAIL2 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (ftest1) REFERENCES PKTABLE(ptest2));
ERROR: column "ptest2" referenced in foreign key constraint does not exist
-DROP TABLE FKTABLE_FAIL1;
-ERROR: table "fktable_fail1" does not exist
-DROP TABLE FKTABLE_FAIL2;
-ERROR: table "fktable_fail2" does not exist
+CREATE TABLE FKTABLE_FAIL3 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (tableoid) REFERENCES PKTABLE(someoid));
+ERROR: system columns cannot be used in foreign keys
+CREATE TABLE FKTABLE_FAIL4 ( ftest1 oid, CONSTRAINT fkfail1 FOREIGN KEY (ftest1) REFERENCES PKTABLE(tableoid));
+ERROR: system columns cannot be used in foreign keys
DROP TABLE PKTABLE;
-- Test for referencing column number smaller than referenced constraint
CREATE TABLE PKTABLE (ptest1 int, ptest2 int, UNIQUE(ptest1, ptest2));
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 90f6db7f21..22e177f89b 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -490,12 +490,13 @@ SELECT * FROM FKTABLE ORDER BY id;
DROP TABLE FKTABLE;
DROP TABLE PKTABLE;
-CREATE TABLE PKTABLE (ptest1 int PRIMARY KEY);
+-- Test some invalid FK definitions
+CREATE TABLE PKTABLE (ptest1 int PRIMARY KEY, someoid oid);
CREATE TABLE FKTABLE_FAIL1 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (ftest2) REFERENCES PKTABLE);
CREATE TABLE FKTABLE_FAIL2 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (ftest1) REFERENCES PKTABLE(ptest2));
+CREATE TABLE FKTABLE_FAIL3 ( ftest1 int, CONSTRAINT fkfail1 FOREIGN KEY (tableoid) REFERENCES PKTABLE(someoid));
+CREATE TABLE FKTABLE_FAIL4 ( ftest1 oid, CONSTRAINT fkfail1 FOREIGN KEY (ftest1) REFERENCES PKTABLE(tableoid));
-DROP TABLE FKTABLE_FAIL1;
-DROP TABLE FKTABLE_FAIL2;
DROP TABLE PKTABLE;
-- Test for referencing column number smaller than referenced constraint