Unbreak overflow test for attinhcount/coninhcount
authorÁlvaro Herrera <[email protected]>
Thu, 10 Oct 2024 15:41:01 +0000 (17:41 +0200)
committerÁlvaro Herrera <[email protected]>
Thu, 10 Oct 2024 15:41:01 +0000 (17:41 +0200)
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 <[email protected]>
Co-authored-by: 何建 (jian he) <[email protected]>
Reviewed-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/202410081611[email protected]

src/backend/catalog/heap.c
src/backend/catalog/index.c
src/backend/catalog/pg_constraint.c
src/backend/commands/tablecmds.c
src/include/catalog/catversion.h
src/include/catalog/heap.h
src/include/catalog/pg_constraint.h
src/include/nodes/parsenodes.h

index 78e59384d1c1ed52aa08882705544be6b981811e..0078a12f26e9d4910e22cdb38a8c85deea2c5cc2 100644 (file)
@@ -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"));
index 6084dfa97cbeb621f7abcf228cb5134d48227009..12822d0b1400957e304955eff83168c55a20d9d8 100644 (file)
@@ -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;
index 1e2df031a848f81e504af3d2ea7deb763c647eb8..54f3fb50a5797bf6db246a627b2db497f542bdb9 100644 (file)
@@ -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);
index af8c05b91f13af16cbb827a18d9bf49c84200045..1ccc80087c3980e683de428ceebe7040590c1810 100644 (file)
@@ -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"));
index 8cdb8c5d6c642d98ec90da61c8d855b4fef5e4e9..8cb918d9116ea3050e46d68e7d37633a4826664d 100644 (file)
@@ -57,6 +57,6 @@
  */
 
 /*                                                     yyyymmddN */
-#define CATALOG_VERSION_NO     202410111
+#define CATALOG_VERSION_NO     202410112
 
 #endif
index c512824cd1cc1027cc07c2b6d6a70c3cbb07a606..d6a2c791290c0bc2fa510f5fa3f39bc047cc1b02 100644 (file)
@@ -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;
index 115217a61620df54205daa4e4fa9c71133a2ee01..35788315bc4b71081c89309cff8a1cbcd4eb89ce 100644 (file)
@@ -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);
index 1c314cd9074d0e4dfcd4ba5305f1fd1582e48737..5b62df32733f4184055351590035ff0201c1a76e 100644 (file)
@@ -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 */