From fd64ed60b62697984bb69a09a3ae19fbe2905eb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= Date: Thu, 10 Oct 2024 17:41:01 +0200 Subject: [PATCH] Unbreak overflow test for attinhcount/coninhcount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 90189eefc1e1 narrowed pg_attribute.attinhcount and pg_constraint.coninhcount from 32 to 16 bits, but kept other related structs with 32-bit wide fields: ColumnDef and CookedConstraint contain an int 'inhcount' field which is itself checked for overflow on increments, but there's no check that the values aren't above INT16_MAX before assigning to the catalog columns. This means that a creative user can get a inconsistent table definition and override some protections. Fix it by changing those other structs to also use int16. Also, modernize style by using pg_add_s16_overflow for overflow testing instead of checking for negative values. We also have Constraint.inhcount, which is here removed completely. This was added by commit b0e96f311985 and not removed by its revert at 6f8bb7c1e961. It is not needed by the upcoming not-null constraints patch. This is mostly academic, so we agreed not to backpatch to avoid ABI problems. Bump catversion because of the changes to parse nodes. Co-authored-by: Álvaro Herrera Co-authored-by: 何建 (jian he) Reviewed-by: Tom Lane Discussion: https://postgr.es/m/202410081611.up4iyofb5ie7@alvherre.pgsql --- src/backend/catalog/heap.c | 10 ++++----- src/backend/catalog/index.c | 2 +- src/backend/catalog/pg_constraint.c | 8 ++++--- src/backend/commands/tablecmds.c | 34 +++++++++++++++-------------- src/include/catalog/catversion.h | 2 +- src/include/catalog/heap.h | 2 +- src/include/catalog/pg_constraint.h | 2 +- src/include/nodes/parsenodes.h | 4 +--- 8 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 78e59384d1c..0078a12f26e 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -102,7 +102,7 @@ static ObjectAddress AddNewRelationType(const char *typeName, Oid new_array_type); static void RelationRemoveInheritance(Oid relid); static Oid StoreRelCheck(Relation rel, const char *ccname, Node *expr, - bool is_validated, bool is_local, int inhcount, + bool is_validated, bool is_local, int16 inhcount, bool is_no_inherit, bool is_internal); static void StoreConstraints(Relation rel, List *cooked_constraints, bool is_internal); @@ -2072,7 +2072,7 @@ SetAttrMissing(Oid relid, char *attname, char *value) */ static Oid StoreRelCheck(Relation rel, const char *ccname, Node *expr, - bool is_validated, bool is_local, int inhcount, + bool is_validated, bool is_local, int16 inhcount, bool is_no_inherit, bool is_internal) { char *ccbin; @@ -2624,10 +2624,8 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr, { if (is_local) con->conislocal = true; - else - con->coninhcount++; - - if (con->coninhcount < 0) + else if (pg_add_s16_overflow(con->coninhcount, 1, + &con->coninhcount)) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 6084dfa97cb..12822d0b140 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1900,7 +1900,7 @@ index_constraint_create(Relation heapRelation, bool islocal; bool noinherit; bool is_without_overlaps; - int inhcount; + int16 inhcount; deferrable = (constr_flags & INDEX_CONSTR_CREATE_DEFERRABLE) != 0; initdeferred = (constr_flags & INDEX_CONSTR_CREATE_INIT_DEFERRED) != 0; diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 1e2df031a84..54f3fb50a57 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -27,6 +27,7 @@ #include "catalog/pg_operator.h" #include "catalog/pg_type.h" #include "commands/defrem.h" +#include "common/int.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/fmgroids.h" @@ -74,7 +75,7 @@ CreateConstraintEntry(const char *constraintName, Node *conExpr, const char *conBin, bool conIsLocal, - int conInhCount, + int16 conInhCount, bool conNoInherit, bool conPeriod, bool is_internal) @@ -849,11 +850,12 @@ ConstraintSetParentConstraint(Oid childConstrId, childConstrId); constrForm->conislocal = false; - constrForm->coninhcount++; - if (constrForm->coninhcount < 0) + if (pg_add_s16_overflow(constrForm->coninhcount, 1, + &constrForm->coninhcount)) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); + constrForm->conparentid = parentConstrId; CatalogTupleUpdate(constrRel, &tuple->t_self, newtup); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index af8c05b91f1..1ccc80087c3 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -66,6 +66,7 @@ #include "commands/typecmds.h" #include "commands/user.h" #include "commands/vacuum.h" +#include "common/int.h" #include "executor/executor.h" #include "foreign/fdwapi.h" #include "foreign/foreign.h" @@ -3044,8 +3045,8 @@ MergeCheckConstraint(List *constraints, const char *name, Node *expr) if (equal(expr, ccon->expr)) { /* OK to merge constraint with existing */ - ccon->inhcount++; - if (ccon->inhcount < 0) + if (pg_add_s16_overflow(ccon->inhcount, 1, + &ccon->inhcount)) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); @@ -3347,8 +3348,8 @@ MergeInheritedAttribute(List *inh_columns, * Default and other constraints are handled by the caller. */ - prevdef->inhcount++; - if (prevdef->inhcount < 0) + if (pg_add_s16_overflow(prevdef->inhcount, 1, + &prevdef->inhcount)) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); @@ -7089,8 +7090,8 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, get_collation_name(childatt->attcollation)))); /* Bump the existing child att's inhcount */ - childatt->attinhcount++; - if (childatt->attinhcount < 0) + if (pg_add_s16_overflow(childatt->attinhcount, 1, + &childatt->attinhcount)) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); @@ -10170,7 +10171,7 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, Oid constrOid; char *conname; bool conislocal; - int coninhcount; + int16 coninhcount; bool connoinherit; Oid deleteTriggerOid, updateTriggerOid; @@ -10549,9 +10550,9 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, NULL, NULL, NULL, - false, - 1, - false, + false, /* conIsLocal */ + 1, /* conInhCount */ + false, /* conNoInherit */ with_period, /* conPeriod */ false); @@ -11076,8 +11077,8 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) NULL, NULL, NULL, - false, /* islocal */ - 1, /* inhcount */ + false, /* conIsLocal */ + 1, /* conInhCount */ false, /* conNoInherit */ with_period, /* conPeriod */ true); @@ -15944,8 +15945,8 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel, bool ispart * OK, bump the child column's inheritance count. (If we fail * later on, this change will just roll back.) */ - child_att->attinhcount++; - if (child_att->attinhcount < 0) + if (pg_add_s16_overflow(child_att->attinhcount, 1, + &child_att->attinhcount)) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); @@ -16075,8 +16076,9 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) */ child_copy = heap_copytuple(child_tuple); child_con = (Form_pg_constraint) GETSTRUCT(child_copy); - child_con->coninhcount++; - if (child_con->coninhcount < 0) + + if (pg_add_s16_overflow(child_con->coninhcount, 1, + &child_con->coninhcount)) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 8cdb8c5d6c6..8cb918d9116 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -57,6 +57,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 202410111 +#define CATALOG_VERSION_NO 202410112 #endif diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h index c512824cd1c..d6a2c791290 100644 --- a/src/include/catalog/heap.h +++ b/src/include/catalog/heap.h @@ -41,7 +41,7 @@ typedef struct CookedConstraint Node *expr; /* transformed default or check expr */ bool skip_validation; /* skip validation? (only for CHECK) */ bool is_local; /* constraint has local (non-inherited) def */ - int inhcount; /* number of times constraint is inherited */ + int16 inhcount; /* number of times constraint is inherited */ bool is_no_inherit; /* constraint has local def and cannot be * inherited */ } CookedConstraint; diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h index 115217a6162..35788315bc4 100644 --- a/src/include/catalog/pg_constraint.h +++ b/src/include/catalog/pg_constraint.h @@ -245,7 +245,7 @@ extern Oid CreateConstraintEntry(const char *constraintName, Node *conExpr, const char *conBin, bool conIsLocal, - int conInhCount, + int16 conInhCount, bool conNoInherit, bool conPeriod, bool is_internal); diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 1c314cd9074..5b62df32733 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -728,7 +728,7 @@ typedef struct ColumnDef char *colname; /* name of column */ TypeName *typeName; /* type of column */ char *compression; /* compression method for column */ - int inhcount; /* number of times column is inherited */ + int16 inhcount; /* number of times column is inherited */ bool is_local; /* column has local (non-inherited) def'n */ bool is_not_null; /* NOT NULL constraint specified? */ bool is_from_type; /* column definition came from table type */ @@ -2754,8 +2754,6 @@ typedef struct Constraint char *cooked_expr; /* CHECK or DEFAULT expression, as * nodeToString representation */ char generated_when; /* ALWAYS or BY DEFAULT */ - int inhcount; /* initial inheritance count to apply, for - * "raw" NOT NULL constraints */ bool nulls_not_distinct; /* null treatment for UNIQUE constraints */ List *keys; /* String nodes naming referenced key * column(s); for UNIQUE/PK/NOT NULL */