From eb51af71f241e8cb199790dee9ad246bb36b3287 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 28 Mar 2011 15:44:54 -0400 Subject: [PATCH] Prevent a rowtype from being included in itself. Eventually we might be able to allow that, but it's not clear how many places need to be fixed to prevent infinite recursion when there's a direct or indirect inclusion of a rowtype in itself. One such place is CheckAttributeType(), which will recurse to stack overflow in cases such as those exhibited in bug #5950 from Alex Perepelica. If we were sure it was the only such place, we could easily modify the code added by this patch to stop the recursion without a complaint ... but it probably isn't the only such place. Hence, throw error until such time as someone is excited enough about this type of usage to put work into making it safe. Back-patch as far as 8.3. 8.2 doesn't have the recursive call in CheckAttributeType in the first place, so I see no need to add code there in the absence of clear evidence of a problem elsewhere. --- src/backend/catalog/heap.c | 49 ++++++++++++++++++++--- src/backend/catalog/index.c | 4 +- src/backend/commands/tablecmds.c | 8 +++- src/include/catalog/heap.h | 4 +- src/test/regress/expected/alter_table.out | 12 ++++++ src/test/regress/sql/alter_table.sql | 9 +++++ 6 files changed, 76 insertions(+), 10 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 567eb7fd6e..5d25ce9ec8 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -431,6 +431,7 @@ CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, CheckAttributeType(NameStr(tupdesc->attrs[i]->attname), tupdesc->attrs[i]->atttypid, tupdesc->attrs[i]->attcollation, + NIL, /* assume we're creating a new rowtype */ allow_system_table_mods); } } @@ -439,15 +440,25 @@ CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, * CheckAttributeType * * Verify that the proposed datatype of an attribute is legal. - * This is needed because there are types (and pseudo-types) + * This is needed mainly because there are types (and pseudo-types) * in the catalogs that we do not support as elements of real tuples. + * We also check some other properties required of a table column. + * + * If the attribute is being proposed for addition to an existing table or + * composite type, pass a one-element list of the rowtype OID as + * containing_rowtypes. When checking a to-be-created rowtype, it's + * sufficient to pass NIL, because there could not be any recursive reference + * to a not-yet-existing rowtype. * -------------------------------- */ void -CheckAttributeType(const char *attname, Oid atttypid, Oid attcollation, +CheckAttributeType(const char *attname, + Oid atttypid, Oid attcollation, + List *containing_rowtypes, bool allow_system_table_mods) { char att_typtype = get_typtype(atttypid); + Oid att_typelem; if (atttypid == UNKNOWNOID) { @@ -485,6 +496,20 @@ CheckAttributeType(const char *attname, Oid atttypid, Oid attcollation, TupleDesc tupdesc; int i; + /* + * Check for self-containment. Eventually we might be able to allow + * this (just return without complaint, if so) but it's not clear how + * many other places would require anti-recursion defenses before it + * would be safe to allow tables to contain their own rowtype. + */ + if (list_member_oid(containing_rowtypes, atttypid)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("composite type %s cannot be made a member of itself", + format_type_be(atttypid)))); + + containing_rowtypes = lcons_oid(atttypid, containing_rowtypes); + relation = relation_open(get_typ_typrelid(atttypid), AccessShareLock); tupdesc = RelationGetDescr(relation); @@ -495,19 +520,31 @@ CheckAttributeType(const char *attname, Oid atttypid, Oid attcollation, if (attr->attisdropped) continue; - CheckAttributeType(NameStr(attr->attname), attr->atttypid, attr->attcollation, + CheckAttributeType(NameStr(attr->attname), + attr->atttypid, attr->attcollation, + containing_rowtypes, allow_system_table_mods); } relation_close(relation, AccessShareLock); + + containing_rowtypes = list_delete_first(containing_rowtypes); + } + else if (OidIsValid((att_typelem = get_element_type(atttypid)))) + { + /* + * Must recurse into array types, too, in case they are composite. + */ + CheckAttributeType(attname, att_typelem, attcollation, + containing_rowtypes, + allow_system_table_mods); } /* * This might not be strictly invalid per SQL standard, but it is - * pretty useless, and it cannot be dumped, so we must disallow - * it. + * pretty useless, and it cannot be dumped, so we must disallow it. */ - if (type_is_collatable(atttypid) && !OidIsValid(attcollation)) + if (!OidIsValid(attcollation) && type_is_collatable(atttypid)) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("no collation was derived for column \"%s\" with collatable type %s", diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 7a2629ecd1..679255a199 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -403,7 +403,9 @@ ConstructTupleDescriptor(Relation heapRelation, * whether a table column is of a safe type (which is why we * needn't check for the non-expression case). */ - CheckAttributeType(NameStr(to->attname), to->atttypid, to->attcollation, false); + CheckAttributeType(NameStr(to->attname), + to->atttypid, to->attcollation, + NIL, false); } /* diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d7573b8ef5..737ab1a7bc 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -4221,7 +4221,9 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel, collOid = GetColumnDefCollation(NULL, colDef, typeOid); /* make sure datatype is legal for a column */ - CheckAttributeType(colDef->colname, typeOid, collOid, false); + CheckAttributeType(colDef->colname, typeOid, collOid, + list_make1_oid(rel->rd_rel->reltype), + false); /* construct new attribute's pg_attribute entry */ attribute.attrelid = myrelid; @@ -6536,7 +6538,9 @@ ATPrepAlterColumnType(List **wqueue, targetcollid = GetColumnDefCollation(NULL, def, targettype); /* make sure datatype is legal for a column */ - CheckAttributeType(colName, targettype, targetcollid, false); + CheckAttributeType(colName, targettype, targetcollid, + list_make1_oid(rel->rd_rel->reltype), + false); if (tab->relkind == RELKIND_RELATION) { diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h index 5f9a864be5..463aff0358 100644 --- a/src/include/catalog/heap.h +++ b/src/include/catalog/heap.h @@ -117,7 +117,9 @@ extern Form_pg_attribute SystemAttributeByName(const char *attname, extern void CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, bool allow_system_table_mods); -extern void CheckAttributeType(const char *attname, Oid atttypid, Oid attcollation, +extern void CheckAttributeType(const char *attname, + Oid atttypid, Oid attcollation, + List *containing_rowtypes, bool allow_system_table_mods); #endif /* HEAP_H */ diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index d5a59d5d1d..578f94bc64 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1493,6 +1493,18 @@ create table tab1 (a int, b text); create table tab2 (x int, y tab1); alter table tab1 alter column b type varchar; -- fails ERROR: cannot alter table "tab1" because column "tab2"."y" uses its rowtype +-- disallow recursive containment of row types +create temp table recur1 (f1 int); +alter table recur1 add column f2 recur1; -- fails +ERROR: composite type recur1 cannot be made a member of itself +alter table recur1 add column f2 recur1[]; -- fails +ERROR: composite type recur1 cannot be made a member of itself +create temp table recur2 (f1 int, f2 recur1); +alter table recur1 add column f2 recur2; -- fails +ERROR: composite type recur1 cannot be made a member of itself +alter table recur1 add column f2 int; +alter table recur1 alter column f2 type recur2; -- fails +ERROR: composite type recur1 cannot be made a member of itself -- -- lock levels -- diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 6531a9f162..54dbb8eaf9 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1112,6 +1112,15 @@ create table tab1 (a int, b text); create table tab2 (x int, y tab1); alter table tab1 alter column b type varchar; -- fails +-- disallow recursive containment of row types +create temp table recur1 (f1 int); +alter table recur1 add column f2 recur1; -- fails +alter table recur1 add column f2 recur1[]; -- fails +create temp table recur2 (f1 int, f2 recur1); +alter table recur1 add column f2 recur2; -- fails +alter table recur1 add column f2 int; +alter table recur1 alter column f2 type recur2; -- fails + -- -- lock levels -- -- 2.40.0