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
<https://scan6.coverity.com/eventId=32356039-3&modelId=32356039-0&fileInstanceId=112435213&filePath=%2Fdll%2Fpostgres%2Fsrc%2Fbackend%2Fexecutor%2FexecUtils.c&fileStart=1230&fileEnd=1255>
]
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
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 inHi,
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 inHi,
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 thisResultRelInfo
* 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]https://github.com/postgres/postgres/commit/54e51dcde03e5c746e8de6243c69fafdc8d0ec7a.
regards,
Ranier Vilela
[1]: https://github.com/postgres/postgres/commit/54e51dcde03e5c746e8de6243c69fafdc8d0ec7a
https://github.com/postgres/postgres/commit/54e51dcde03e5c746e8de6243c69fafdc8d0ec7a