Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 8e56684

Browse filesBrowse files
hlinnakahlinnaka
committed
Fix permission checks on constraint violation errors on partitions.
If a cross-partition UPDATE violates a constraint on the target partition, and the columns in the new partition are in different physical order than in the parent, the error message can reveal columns that the user does not have SELECT permission on. A similar bug was fixed earlier in commit 804b6b6. The cause of the bug is that the callers of the ExecBuildSlotValueDescription() function got confused when constructing the list of modified columns. If the tuple was routed from a parent, we converted the tuple to the parent's format, but the list of modified columns was grabbed directly from the child's RTE entry. ExecUpdateLockMode() had a similar issue. That lead to confusion on which columns are key columns, leading to wrong tuple lock being taken on tables referenced by foreign keys, when a row is updated with INSERT ON CONFLICT UPDATE. A new isolation test is added for that corner case. With this patch, the ri_RangeTableIndex field is no longer set for partitions that don't have an entry in the range table. Previously, it was set to the RTE entry of the parent relation, but that was confusing. NOTE: This modifies the ResultRelInfo struct, replacing the ri_PartitionRoot field with ri_RootResultRelInfo. That's a bit risky to backpatch, because it breaks any extensions accessing the field. The change that ri_RangeTableIndex is not set for partitions could potentially break extensions, too. The ResultRelInfos are visible to FDWs at least, and this patch required small changes to postgres_fdw. Nevertheless, this seem like the least bad option. I don't think these fields widely used in extensions; I don't think there are FDWs out there that uses the FDW "direct update" API, other than postgres_fdw. If there is, you will get a compilation error, so hopefully it is caught quickly. Backpatch to 11, where support for both cross-partition UPDATEs, and unique indexes on partitioned tables, were added. Reviewed-by: Amit Langote Security: CVE-2021-3393
1 parent b4199a9 commit 8e56684
Copy full SHA for 8e56684

File tree

Expand file treeCollapse file tree

17 files changed

+370
-89
lines changed
Filter options
Expand file treeCollapse file tree

17 files changed

+370
-89
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/postgres_fdw.c
+15-5Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1922,7 +1922,7 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
19221922
PgFdwModifyState *fmstate;
19231923
ModifyTable *plan = castNode(ModifyTable, mtstate->ps.plan);
19241924
EState *estate = mtstate->ps.state;
1925-
Index resultRelation = resultRelInfo->ri_RangeTableIndex;
1925+
Index resultRelation;
19261926
Relation rel = resultRelInfo->ri_RelationDesc;
19271927
RangeTblEntry *rte;
19281928
TupleDesc tupdesc = RelationGetDescr(rel);
@@ -1973,17 +1973,20 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
19731973
}
19741974

19751975
/*
1976-
* If the foreign table is a partition, we need to create a new RTE
1976+
* If the foreign table is a partition that doesn't have a corresponding
1977+
* RTE entry, we need to create a new RTE
19771978
* describing the foreign table for use by deparseInsertSql and
19781979
* create_foreign_modify() below, after first copying the parent's RTE and
19791980
* modifying some fields to describe the foreign partition to work on.
19801981
* However, if this is invoked by UPDATE, the existing RTE may already
19811982
* correspond to this partition if it is one of the UPDATE subplan target
19821983
* rels; in that case, we can just use the existing RTE as-is.
19831984
*/
1984-
rte = exec_rt_fetch(resultRelation, estate);
1985-
if (rte->relid != RelationGetRelid(rel))
1985+
if (resultRelInfo->ri_RangeTableIndex == 0)
19861986
{
1987+
ResultRelInfo *rootResultRelInfo = resultRelInfo->ri_RootResultRelInfo;
1988+
1989+
rte = exec_rt_fetch(rootResultRelInfo->ri_RangeTableIndex, estate);
19871990
rte = copyObject(rte);
19881991
rte->relid = RelationGetRelid(rel);
19891992
rte->relkind = RELKIND_FOREIGN_TABLE;
@@ -1995,8 +1998,15 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
19951998
* Vars contained in those expressions.
19961999
*/
19972000
if (plan && plan->operation == CMD_UPDATE &&
1998-
resultRelation == plan->rootRelation)
2001+
rootResultRelInfo->ri_RangeTableIndex == plan->rootRelation)
19992002
resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex;
2003+
else
2004+
resultRelation = rootResultRelInfo->ri_RangeTableIndex;
2005+
}
2006+
else
2007+
{
2008+
resultRelation = resultRelInfo->ri_RangeTableIndex;
2009+
rte = exec_rt_fetch(resultRelation, estate);
20002010
}
20012011

20022012
/* Construct the SQL command string. */

‎src/backend/access/common/tupconvert.c

Copy file name to clipboardExpand all lines: src/backend/access/common/tupconvert.c
+51Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,57 @@ execute_attr_map_slot(AttrMap *attrMap,
226226
return out_slot;
227227
}
228228

229+
/*
230+
* Perform conversion of bitmap of columns according to the map.
231+
*
232+
* The input and output bitmaps are offset by
233+
* FirstLowInvalidHeapAttributeNumber to accommodate system cols, like the
234+
* column-bitmaps in RangeTblEntry.
235+
*/
236+
Bitmapset *
237+
execute_attr_map_cols(AttrMap *attrMap, Bitmapset *in_cols)
238+
{
239+
Bitmapset *out_cols;
240+
int out_attnum;
241+
242+
/* fast path for the common trivial case */
243+
if (in_cols == NULL)
244+
return NULL;
245+
246+
/*
247+
* For each output column, check which input column it corresponds to.
248+
*/
249+
out_cols = NULL;
250+
251+
for (out_attnum = FirstLowInvalidHeapAttributeNumber + 1;
252+
out_attnum <= attrMap->maplen;
253+
out_attnum++)
254+
{
255+
int in_attnum;
256+
257+
if (out_attnum < 0)
258+
{
259+
/* System column. No mapping. */
260+
in_attnum = out_attnum;
261+
}
262+
else if (out_attnum == 0)
263+
continue;
264+
else
265+
{
266+
/* normal user column */
267+
in_attnum = attrMap->attnums[out_attnum - 1];
268+
269+
if (in_attnum == 0)
270+
continue;
271+
}
272+
273+
if (bms_is_member(in_attnum - FirstLowInvalidHeapAttributeNumber, in_cols))
274+
out_cols = bms_add_member(out_cols, out_attnum - FirstLowInvalidHeapAttributeNumber);
275+
}
276+
277+
return out_cols;
278+
}
279+
229280
/*
230281
* Free a TupleConversionMap structure.
231282
*/

‎src/backend/commands/copy.c

Copy file name to clipboardExpand all lines: src/backend/commands/copy.c
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2804,6 +2804,7 @@ CopyFrom(CopyState cstate)
28042804
mtstate->ps.state = estate;
28052805
mtstate->operation = CMD_INSERT;
28062806
mtstate->resultRelInfo = estate->es_result_relations;
2807+
mtstate->rootResultRelInfo = estate->es_result_relations;
28072808

28082809
if (resultRelInfo->ri_FdwRoutine != NULL &&
28092810
resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)

‎src/backend/commands/explain.c

Copy file name to clipboardExpand all lines: src/backend/commands/explain.c
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3681,7 +3681,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors,
36813681
/* Should we explicitly label target relations? */
36823682
labeltargets = (mtstate->mt_nplans > 1 ||
36833683
(mtstate->mt_nplans == 1 &&
3684-
mtstate->resultRelInfo->ri_RangeTableIndex != node->nominalRelation));
3684+
mtstate->resultRelInfo[0].ri_RangeTableIndex != node->nominalRelation));
36853685

36863686
if (labeltargets)
36873687
ExplainOpenGroup("Target Tables", "Target Tables", false, es);

‎src/backend/commands/trigger.c

Copy file name to clipboardExpand all lines: src/backend/commands/trigger.c
+10-14Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -69,16 +69,6 @@ int SessionReplicationRole = SESSION_REPLICATION_ROLE_ORIGIN;
6969
/* How many levels deep into trigger execution are we? */
7070
static int MyTriggerDepth = 0;
7171

72-
/*
73-
* Note that similar macros also exist in executor/execMain.c. There does not
74-
* appear to be any good header to put them into, given the structures that
75-
* they use, so we let them be duplicated. Be sure to update all if one needs
76-
* to be changed, however.
77-
*/
78-
#define GetAllUpdatedColumns(relinfo, estate) \
79-
(bms_union(exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->updatedCols, \
80-
exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->extraUpdatedCols))
81-
8272
/* Local function prototypes */
8373
static void SetTriggerFlags(TriggerDesc *trigdesc, Trigger *trigger);
8474
static bool GetTupleForTrigger(EState *estate,
@@ -2570,7 +2560,10 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
25702560
CMD_UPDATE))
25712561
return;
25722562

2573-
updatedCols = GetAllUpdatedColumns(relinfo, estate);
2563+
/* statement-level triggers operate on the parent table */
2564+
Assert(relinfo->ri_RootResultRelInfo == NULL);
2565+
2566+
updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
25742567

25752568
LocTriggerData.type = T_TriggerData;
25762569
LocTriggerData.tg_event = TRIGGER_EVENT_UPDATE |
@@ -2611,10 +2604,13 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
26112604
{
26122605
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
26132606

2607+
/* statement-level triggers operate on the parent table */
2608+
Assert(relinfo->ri_RootResultRelInfo == NULL);
2609+
26142610
if (trigdesc && trigdesc->trig_update_after_statement)
26152611
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE,
26162612
false, NULL, NULL, NIL,
2617-
GetAllUpdatedColumns(relinfo, estate),
2613+
ExecGetAllUpdatedCols(relinfo, estate),
26182614
transition_capture);
26192615
}
26202616

@@ -2684,7 +2680,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
26842680
TRIGGER_EVENT_ROW |
26852681
TRIGGER_EVENT_BEFORE;
26862682
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
2687-
updatedCols = GetAllUpdatedColumns(relinfo, estate);
2683+
updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
26882684
LocTriggerData.tg_updatedcols = updatedCols;
26892685
for (i = 0; i < trigdesc->numtriggers; i++)
26902686
{
@@ -2785,7 +2781,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
27852781

27862782
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE,
27872783
true, oldslot, newslot, recheckIndexes,
2788-
GetAllUpdatedColumns(relinfo, estate),
2784+
ExecGetAllUpdatedCols(relinfo, estate),
27892785
transition_capture);
27902786
}
27912787
}

‎src/backend/executor/execMain.c

Copy file name to clipboardExpand all lines: src/backend/executor/execMain.c
+39-48Lines changed: 39 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -100,20 +100,6 @@ static char *ExecBuildSlotValueDescription(Oid reloid,
100100
int maxfieldlen);
101101
static void EvalPlanQualStart(EPQState *epqstate, Plan *planTree);
102102

103-
/*
104-
* Note that GetAllUpdatedColumns() also exists in commands/trigger.c. There does
105-
* not appear to be any good header to put it into, given the structures that
106-
* it uses, so we let them be duplicated. Be sure to update both if one needs
107-
* to be changed, however.
108-
*/
109-
#define GetInsertedColumns(relinfo, estate) \
110-
(exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->insertedCols)
111-
#define GetUpdatedColumns(relinfo, estate) \
112-
(exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->updatedCols)
113-
#define GetAllUpdatedColumns(relinfo, estate) \
114-
(bms_union(exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->updatedCols, \
115-
exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->extraUpdatedCols))
116-
117103
/* end of local decls */
118104

119105

@@ -1277,7 +1263,7 @@ void
12771263
InitResultRelInfo(ResultRelInfo *resultRelInfo,
12781264
Relation resultRelationDesc,
12791265
Index resultRelationIndex,
1280-
Relation partition_root,
1266+
ResultRelInfo *partition_root_rri,
12811267
int instrument_options)
12821268
{
12831269
List *partition_check = NIL;
@@ -1342,7 +1328,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
13421328
partition_check = RelationGetPartitionQual(resultRelationDesc);
13431329

13441330
resultRelInfo->ri_PartitionCheck = partition_check;
1345-
resultRelInfo->ri_PartitionRoot = partition_root;
1331+
resultRelInfo->ri_RootResultRelInfo = partition_root_rri;
13461332
resultRelInfo->ri_PartitionInfo = NULL; /* may be set later */
13471333
resultRelInfo->ri_CopyMultiInsertBuffer = NULL;
13481334
}
@@ -1840,13 +1826,14 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
18401826
* back to the root table's rowtype so that val_desc in the error message
18411827
* matches the input tuple.
18421828
*/
1843-
if (resultRelInfo->ri_PartitionRoot)
1829+
if (resultRelInfo->ri_RootResultRelInfo)
18441830
{
1831+
ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo;
18451832
TupleDesc old_tupdesc;
18461833
AttrMap *map;
18471834

1848-
root_relid = RelationGetRelid(resultRelInfo->ri_PartitionRoot);
1849-
tupdesc = RelationGetDescr(resultRelInfo->ri_PartitionRoot);
1835+
root_relid = RelationGetRelid(rootrel->ri_RelationDesc);
1836+
tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
18501837

18511838
old_tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
18521839
/* a reverse map */
@@ -1859,16 +1846,17 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
18591846
if (map != NULL)
18601847
slot = execute_attr_map_slot(map, slot,
18611848
MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
1849+
modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
1850+
ExecGetUpdatedCols(rootrel, estate));
18621851
}
18631852
else
18641853
{
18651854
root_relid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
18661855
tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
1856+
modifiedCols = bms_union(ExecGetInsertedCols(resultRelInfo, estate),
1857+
ExecGetUpdatedCols(resultRelInfo, estate));
18671858
}
18681859

1869-
modifiedCols = bms_union(GetInsertedColumns(resultRelInfo, estate),
1870-
GetUpdatedColumns(resultRelInfo, estate));
1871-
18721860
val_desc = ExecBuildSlotValueDescription(root_relid,
18731861
slot,
18741862
tupdesc,
@@ -1901,8 +1889,6 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
19011889
TupleDesc tupdesc = RelationGetDescr(rel);
19021890
TupleConstr *constr = tupdesc->constr;
19031891
Bitmapset *modifiedCols;
1904-
Bitmapset *insertedCols;
1905-
Bitmapset *updatedCols;
19061892

19071893
Assert(constr || resultRelInfo->ri_PartitionCheck);
19081894

@@ -1928,12 +1914,12 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
19281914
* rowtype so that val_desc shown error message matches the
19291915
* input tuple.
19301916
*/
1931-
if (resultRelInfo->ri_PartitionRoot)
1917+
if (resultRelInfo->ri_RootResultRelInfo)
19321918
{
1919+
ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo;
19331920
AttrMap *map;
19341921

1935-
rel = resultRelInfo->ri_PartitionRoot;
1936-
tupdesc = RelationGetDescr(rel);
1922+
tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
19371923
/* a reverse map */
19381924
map = build_attrmap_by_name_if_req(orig_tupdesc,
19391925
tupdesc);
@@ -1945,11 +1931,13 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
19451931
if (map != NULL)
19461932
slot = execute_attr_map_slot(map, slot,
19471933
MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
1934+
modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
1935+
ExecGetUpdatedCols(rootrel, estate));
1936+
rel = rootrel->ri_RelationDesc;
19481937
}
1949-
1950-
insertedCols = GetInsertedColumns(resultRelInfo, estate);
1951-
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
1952-
modifiedCols = bms_union(insertedCols, updatedCols);
1938+
else
1939+
modifiedCols = bms_union(ExecGetInsertedCols(resultRelInfo, estate),
1940+
ExecGetUpdatedCols(resultRelInfo, estate));
19531941
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
19541942
slot,
19551943
tupdesc,
@@ -1977,13 +1965,13 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
19771965
Relation orig_rel = rel;
19781966

19791967
/* See the comment above. */
1980-
if (resultRelInfo->ri_PartitionRoot)
1968+
if (resultRelInfo->ri_RootResultRelInfo)
19811969
{
1970+
ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo;
19821971
TupleDesc old_tupdesc = RelationGetDescr(rel);
19831972
AttrMap *map;
19841973

1985-
rel = resultRelInfo->ri_PartitionRoot;
1986-
tupdesc = RelationGetDescr(rel);
1974+
tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
19871975
/* a reverse map */
19881976
map = build_attrmap_by_name_if_req(old_tupdesc,
19891977
tupdesc);
@@ -1995,11 +1983,13 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
19951983
if (map != NULL)
19961984
slot = execute_attr_map_slot(map, slot,
19971985
MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
1986+
modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
1987+
ExecGetUpdatedCols(rootrel, estate));
1988+
rel = rootrel->ri_RelationDesc;
19981989
}
1999-
2000-
insertedCols = GetInsertedColumns(resultRelInfo, estate);
2001-
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
2002-
modifiedCols = bms_union(insertedCols, updatedCols);
1990+
else
1991+
modifiedCols = bms_union(ExecGetInsertedCols(resultRelInfo, estate),
1992+
ExecGetUpdatedCols(resultRelInfo, estate));
20031993
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
20041994
slot,
20051995
tupdesc,
@@ -2068,8 +2058,6 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
20682058
{
20692059
char *val_desc;
20702060
Bitmapset *modifiedCols;
2071-
Bitmapset *insertedCols;
2072-
Bitmapset *updatedCols;
20732061

20742062
switch (wco->kind)
20752063
{
@@ -2084,13 +2072,13 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
20842072
*/
20852073
case WCO_VIEW_CHECK:
20862074
/* See the comment in ExecConstraints(). */
2087-
if (resultRelInfo->ri_PartitionRoot)
2075+
if (resultRelInfo->ri_RootResultRelInfo)
20882076
{
2077+
ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo;
20892078
TupleDesc old_tupdesc = RelationGetDescr(rel);
20902079
AttrMap *map;
20912080

2092-
rel = resultRelInfo->ri_PartitionRoot;
2093-
tupdesc = RelationGetDescr(rel);
2081+
tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
20942082
/* a reverse map */
20952083
map = build_attrmap_by_name_if_req(old_tupdesc,
20962084
tupdesc);
@@ -2102,11 +2090,14 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
21022090
if (map != NULL)
21032091
slot = execute_attr_map_slot(map, slot,
21042092
MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
2105-
}
21062093

2107-
insertedCols = GetInsertedColumns(resultRelInfo, estate);
2108-
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
2109-
modifiedCols = bms_union(insertedCols, updatedCols);
2094+
modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
2095+
ExecGetUpdatedCols(rootrel, estate));
2096+
rel = rootrel->ri_RelationDesc;
2097+
}
2098+
else
2099+
modifiedCols = bms_union(ExecGetInsertedCols(resultRelInfo, estate),
2100+
ExecGetUpdatedCols(resultRelInfo, estate));
21102101
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
21112102
slot,
21122103
tupdesc,
@@ -2320,7 +2311,7 @@ ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo)
23202311
* been modified, then we can use a weaker lock, allowing for better
23212312
* concurrency.
23222313
*/
2323-
updatedCols = GetAllUpdatedColumns(relinfo, estate);
2314+
updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
23242315
keyCols = RelationGetIndexAttrBitmap(relinfo->ri_RelationDesc,
23252316
INDEX_ATTR_BITMAP_KEY);
23262317

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.