Обсуждение: Possible dereference after null check (src/backend/executor/ExecUtils.c)
Hi,
Per Coverity.
The functions ExecGetInsertedCols and ExecGetUpdatedCols at ExecUtils.c
only are safe to call if the variable "ri_RangeTableIndex" is != 0.
Otherwise a possible Dereference after null check (FORWARD_NULL) can be raised.
Exemple:
void
1718ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
1719 TupleTableSlot *slot,
1720 EState *estate)
1721{
1722 Oid root_relid;
1723 TupleDesc tupdesc;
1724 char *val_desc;
1725 Bitmapset *modifiedCols;
1726
1727 /*
1728 * If the tuple has been routed, it's been converted to the partition's
1729 * rowtype, which might differ from the root table's. We must convert it
1730 * back to the root table's rowtype so that val_desc in the error message
1731 * matches the input tuple.
1732 */
1. Condition resultRelInfo->ri_RootResultRelInfo, taking false branch.
2. var_compare_op: Comparing resultRelInfo->ri_RootResultRelInfo to null implies that resultRelInfo->ri_RootResultRelInfo might be null.
1733 if (resultRelInfo->ri_RootResultRelInfo)
1734 {
1735 ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo;
1736 TupleDesc old_tupdesc;
1737 AttrMap *map;
1738
1739 root_relid = RelationGetRelid(rootrel->ri_RelationDesc);
1740 tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
1741
1742 old_tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
1743 /* a reverse map */
1744 map = build_attrmap_by_name_if_req(old_tupdesc, tupdesc);
1745
1746 /*
1747 * Partition-specific slot's tupdesc can't be changed, so allocate a
1748 * new one.
1749 */
1750 if (map != NULL)
1751 slot = execute_attr_map_slot(map, slot,
1752 MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
1753 modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
1754 ExecGetUpdatedCols(rootrel, estate));
1755 }
1756 else
1757 {
1758 root_relid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
1759 tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
CID 1446241 (#1 of 1): Dereference after null check (FORWARD_NULL)3. var_deref_model: Passing resultRelInfo to ExecGetInsertedCols, which dereferences null resultRelInfo->ri_RootResultRelInfo. [show details]
1760 modifiedCols = bms_union(ExecGetInsertedCols(resultRelInfo, estate),
1761 ExecGetUpdatedCols(resultRelInfo, estate));
1762 }
1763
1764 val_desc = ExecBuildSlotValueDescription(root_relid,
1765 slot,
1766 tupdesc,
1767 modifiedCols,
1768 64);
1769 ereport(ERROR,
1770 (errcode(ERRCODE_CHECK_VIOLATION),
1771 errmsg("new row for relation \"%s\" violates partition constraint",
1772 RelationGetRelationName(resultRelInfo->ri_RelationDesc)),
1773 val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
1774 errtable(resultRelInfo->ri_RelationDesc)));
1775}
regards,
Ranier Viela
Re: Possible dereference after null check (src/backend/executor/ExecUtils.c)
От
Kyotaro Horiguchi
Дата:
At Wed, 10 Feb 2021 19:54:46 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> Hi,
>
> Per Coverity.
>
> The functions ExecGetInsertedCols and ExecGetUpdatedCols at ExecUtils.c
> only are safe to call if the variable "ri_RangeTableIndex" is != 0.
>
> Otherwise a possible Dereference after null check (FORWARD_NULL) can be
> raised.
As it turns out, it's a false positive. And perhaps we don't want to
take action just to satisfy the static code analyzer.
The coment in ExecGetInsertedCols says:
> /*
> * The columns are stored in the range table entry. If this ResultRelInfo
> * doesn't have an entry in the range table (i.e. if it represents a
> * partition routing target), fetch the parent's RTE and map the columns
> * to the order they are in the partition.
> */
> if (relinfo->ri_RangeTableIndex != 0)
> {
This means that any one of the two is always usable here. AFAICS,
actually, ri_RangeTableIndex is non-zero for partitioned (=leaf) and
non-partitoned relations and ri_RootResultRelInfo is non-null for
partitioned (parent or intermediate) relations (since they don't have
a coressponding range table entry).
The only cases where both are 0 and NULL are trigger-use, which is
unrelated to the code path.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Em sex., 12 de fev. de 2021 às 03:28, Kyotaro Horiguchi <horikyota.ntt@gmail.com> escreveu:
At Wed, 10 Feb 2021 19:54:46 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> Hi,
>
> Per Coverity.
>
> The functions ExecGetInsertedCols and ExecGetUpdatedCols at ExecUtils.c
> only are safe to call if the variable "ri_RangeTableIndex" is != 0.
>
> Otherwise a possible Dereference after null check (FORWARD_NULL) can be
> raised.
As it turns out, it's a false positive. And perhaps we don't want to
take action just to satisfy the static code analyzer.
The coment in ExecGetInsertedCols says:
> /*
> * The columns are stored in the range table entry. If this ResultRelInfo
> * doesn't have an entry in the range table (i.e. if it represents a
> * partition routing target), fetch the parent's RTE and map the columns
> * to the order they are in the partition.
> */
> if (relinfo->ri_RangeTableIndex != 0)
> {
This means that any one of the two is always usable here. AFAICS,
actually, ri_RangeTableIndex is non-zero for partitioned (=leaf) and
non-partitoned relations and ri_RootResultRelInfo is non-null for
partitioned (parent or intermediate) relations (since they don't have
a coressponding range table entry).
The only cases where both are 0 and NULL are trigger-use, which is
unrelated to the code path.
This is a case where it would be worth an assertion.
What do you think?
regards,
Ranier Vilela
Em sex., 12 de fev. de 2021 às 13:11, Ranier Vilela <ranier.vf@gmail.com> escreveu:
Em sex., 12 de fev. de 2021 às 03:28, Kyotaro Horiguchi <horikyota.ntt@gmail.com> escreveu:At Wed, 10 Feb 2021 19:54:46 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> Hi,
>
> Per Coverity.
>
> The functions ExecGetInsertedCols and ExecGetUpdatedCols at ExecUtils.c
> only are safe to call if the variable "ri_RangeTableIndex" is != 0.
>
> Otherwise a possible Dereference after null check (FORWARD_NULL) can be
> raised.
As it turns out, it's a false positive. And perhaps we don't want to
take action just to satisfy the static code analyzer.
The coment in ExecGetInsertedCols says:
> /*
> * The columns are stored in the range table entry. If this ResultRelInfo
> * doesn't have an entry in the range table (i.e. if it represents a
> * partition routing target), fetch the parent's RTE and map the columns
> * to the order they are in the partition.
> */
> if (relinfo->ri_RangeTableIndex != 0)
> {
This means that any one of the two is always usable here. AFAICS,
actually, ri_RangeTableIndex is non-zero for partitioned (=leaf) and
non-partitoned relations and ri_RootResultRelInfo is non-null for
partitioned (parent or intermediate) relations (since they don't have
a coressponding range table entry).
The only cases where both are 0 and NULL are trigger-use, which is
unrelated to the code path.This is a case where it would be worth an assertion.What do you think?
Apparently my efforts with Coverity have been worth it.
And together we are helping to keep Postgres more secure.
Although sometimes it is not recognized for that [1].
And together we are helping to keep Postgres more secure.
Although sometimes it is not recognized for that [1].
regards,
Ranier Vilela