Skip to content

Commit cb5868c

Browse 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 1c91af9 commit cb5868c

File tree

18 files changed

+359
-95
lines changed

18 files changed

+359
-95
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2021,7 +2021,7 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
20212021
PgFdwModifyState *fmstate;
20222022
ModifyTable *plan = castNode(ModifyTable, mtstate->ps.plan);
20232023
EState *estate = mtstate->ps.state;
2024-
Index resultRelation = resultRelInfo->ri_RangeTableIndex;
2024+
Index resultRelation;
20252025
Relation rel = resultRelInfo->ri_RelationDesc;
20262026
RangeTblEntry *rte;
20272027
TupleDesc tupdesc = RelationGetDescr(rel);
@@ -2072,17 +2072,20 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
20722072
}
20732073

20742074
/*
2075-
* If the foreign table is a partition, we need to create a new RTE
2075+
* If the foreign table is a partition that doesn't have a corresponding
2076+
* RTE entry, we need to create a new RTE
20762077
* describing the foreign table for use by deparseInsertSql and
20772078
* create_foreign_modify() below, after first copying the parent's RTE and
20782079
* modifying some fields to describe the foreign partition to work on.
20792080
* However, if this is invoked by UPDATE, the existing RTE may already
20802081
* correspond to this partition if it is one of the UPDATE subplan target
20812082
* rels; in that case, we can just use the existing RTE as-is.
20822083
*/
2083-
rte = list_nth(estate->es_range_table, resultRelation - 1);
2084-
if (rte->relid != RelationGetRelid(rel))
2084+
if (resultRelInfo->ri_RangeTableIndex == 0)
20852085
{
2086+
ResultRelInfo *rootResultRelInfo = resultRelInfo->ri_RootResultRelInfo;
2087+
2088+
rte = list_nth(estate->es_range_table, rootResultRelInfo->ri_RangeTableIndex - 1);
20862089
rte = copyObject(rte);
20872090
rte->relid = RelationGetRelid(rel);
20882091
rte->relkind = RELKIND_FOREIGN_TABLE;
@@ -2094,8 +2097,15 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
20942097
* Vars contained in those expressions.
20952098
*/
20962099
if (plan && plan->operation == CMD_UPDATE &&
2097-
resultRelation == plan->nominalRelation)
2100+
rootResultRelInfo->ri_RangeTableIndex == plan->nominalRelation)
20982101
resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex;
2102+
else
2103+
resultRelation = rootResultRelInfo->ri_RangeTableIndex;
2104+
}
2105+
else
2106+
{
2107+
resultRelation = resultRelInfo->ri_RangeTableIndex;
2108+
rte = list_nth(estate->es_range_table, resultRelation - 1);
20992109
}
21002110

21012111
/* Construct the SQL command string. */

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include "postgres.h"
2222

2323
#include "access/htup_details.h"
24+
#include "access/sysattr.h"
2425
#include "access/tupconvert.h"
2526
#include "utils/builtins.h"
2627

@@ -401,6 +402,59 @@ do_convert_tuple(HeapTuple tuple, TupleConversionMap *map)
401402
return heap_form_tuple(map->outdesc, outvalues, outisnull);
402403
}
403404

405+
/*
406+
* Perform conversion of bitmap of columns according to the map.
407+
*
408+
* The input and output bitmaps are offset by
409+
* FirstLowInvalidHeapAttributeNumber to accommodate system cols, like the
410+
* column-bitmaps in RangeTblEntry.
411+
*/
412+
Bitmapset *
413+
execute_attr_map_cols(Bitmapset *in_cols, TupleConversionMap *map)
414+
{
415+
AttrNumber *attrMap = map->attrMap;
416+
int maplen = map->outdesc->natts;
417+
Bitmapset *out_cols;
418+
int out_attnum;
419+
420+
/* fast path for the common trivial case */
421+
if (in_cols == NULL)
422+
return NULL;
423+
424+
/*
425+
* For each output column, check which input column it corresponds to.
426+
*/
427+
out_cols = NULL;
428+
429+
for (out_attnum = FirstLowInvalidHeapAttributeNumber + 1;
430+
out_attnum <= maplen;
431+
out_attnum++)
432+
{
433+
int in_attnum;
434+
435+
if (out_attnum < 0)
436+
{
437+
/* System column. No mapping. */
438+
in_attnum = out_attnum;
439+
}
440+
else if (out_attnum == 0)
441+
continue;
442+
else
443+
{
444+
/* normal user column */
445+
in_attnum = attrMap[out_attnum - 1];
446+
447+
if (in_attnum == 0)
448+
continue;
449+
}
450+
451+
if (bms_is_member(in_attnum - FirstLowInvalidHeapAttributeNumber, in_cols))
452+
out_cols = bms_add_member(out_cols, out_attnum - FirstLowInvalidHeapAttributeNumber);
453+
}
454+
455+
return out_cols;
456+
}
457+
404458
/*
405459
* Free a TupleConversionMap structure.
406460
*/

‎src/backend/commands/copy.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2530,6 +2530,7 @@ CopyFrom(CopyState cstate)
25302530
mtstate->ps.state = estate;
25312531
mtstate->operation = CMD_INSERT;
25322532
mtstate->resultRelInfo = estate->es_result_relations;
2533+
mtstate->rootResultRelInfo = estate->es_result_relations;
25332534

25342535
if (resultRelInfo->ri_FdwRoutine != NULL &&
25352536
resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
@@ -2557,7 +2558,7 @@ CopyFrom(CopyState cstate)
25572558
PartitionTupleRouting *proute;
25582559

25592560
proute = cstate->partition_tuple_routing =
2560-
ExecSetupPartitionTupleRouting(NULL, cstate->rel);
2561+
ExecSetupPartitionTupleRouting(NULL, resultRelInfo);
25612562

25622563
/*
25632564
* If we are capturing transition tuples, they may need to be

‎src/backend/commands/explain.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3137,7 +3137,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors,
31373137
/* Should we explicitly label target relations? */
31383138
labeltargets = (mtstate->mt_nplans > 1 ||
31393139
(mtstate->mt_nplans == 1 &&
3140-
mtstate->resultRelInfo->ri_RangeTableIndex != node->nominalRelation));
3140+
mtstate->resultRelInfo[0].ri_RangeTableIndex != node->nominalRelation));
31413141

31423142
if (labeltargets)
31433143
ExplainOpenGroup("Target Tables", "Target Tables", false, es);

‎src/backend/commands/trigger.c

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -68,15 +68,6 @@ int SessionReplicationRole = SESSION_REPLICATION_ROLE_ORIGIN;
6868
/* How many levels deep into trigger execution are we? */
6969
static int MyTriggerDepth = 0;
7070

71-
/*
72-
* Note that similar macros also exist in executor/execMain.c. There does not
73-
* appear to be any good header to put them into, given the structures that
74-
* they use, so we let them be duplicated. Be sure to update all if one needs
75-
* to be changed, however.
76-
*/
77-
#define GetUpdatedColumns(relinfo, estate) \
78-
(rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->updatedCols)
79-
8071
/* Local function prototypes */
8172
static void ConvertTriggerToFK(CreateTrigStmt *stmt, Oid funcoid);
8273
static void SetTriggerFlags(TriggerDesc *trigdesc, Trigger *trigger);
@@ -2892,7 +2883,7 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
28922883
CMD_UPDATE))
28932884
return;
28942885

2895-
updatedCols = GetUpdatedColumns(relinfo, estate);
2886+
updatedCols = ExecGetUpdatedCols(relinfo, estate);
28962887

28972888
LocTriggerData.type = T_TriggerData;
28982889
LocTriggerData.tg_event = TRIGGER_EVENT_UPDATE |
@@ -2938,10 +2929,13 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
29382929
{
29392930
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
29402931

2932+
/* statement-level triggers operate on the parent table */
2933+
Assert(relinfo->ri_RootResultRelInfo == NULL);
2934+
29412935
if (trigdesc && trigdesc->trig_update_after_statement)
29422936
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE,
29432937
false, NULL, NULL, NIL,
2944-
GetUpdatedColumns(relinfo, estate),
2938+
ExecGetUpdatedCols(relinfo, estate),
29452939
transition_capture);
29462940
}
29472941

@@ -3007,7 +3001,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
30073001
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
30083002
LocTriggerData.tg_oldtable = NULL;
30093003
LocTriggerData.tg_newtable = NULL;
3010-
updatedCols = GetUpdatedColumns(relinfo, estate);
3004+
updatedCols = ExecGetUpdatedCols(relinfo, estate);
30113005
for (i = 0; i < trigdesc->numtriggers; i++)
30123006
{
30133007
Trigger *trigger = &trigdesc->triggers[i];
@@ -3099,7 +3093,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
30993093

31003094
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE,
31013095
true, trigtuple, newtuple, recheckIndexes,
3102-
GetUpdatedColumns(relinfo, estate),
3096+
ExecGetUpdatedCols(relinfo, estate),
31033097
transition_capture);
31043098
if (trigtuple != fdw_trigtuple)
31053099
heap_freetuple(trigtuple);

0 commit comments

Comments
 (0)