BUG #14928: Unchecked SearchSysCacheCopy1() return value

On 2017/11/27 18:13, bianpan2016@163.com wrote:

The following bug has been logged on the website:

Bug reference: 14928
Logged by: Pan Bian
Email address: bianpan2016@163.com
PostgreSQL version: 10.1
Operating system: Linux
Description:

File: postgresql-10.1/src/backend/commands/tablecmds.c
Function: ATExecDetachPartition
Line: 13816

Function SearchSysCacheCopy1() may return a NULL pointer if there is no
enough memory. But in function ATExecDetachPartition(), its return value is
not checked, which may result in NULL dereference (see line 13818).

For your convenience, I copy and paste related codes as follows.

13815 classRel = heap_open(RelationRelationId, RowExclusiveLock);
13816 tuple = SearchSysCacheCopy1(RELOID,
13817
ObjectIdGetDatum(RelationGetRelid(partRel)));
13818 Assert(((Form_pg_class) GETSTRUCT(tuple))->relispartition);
13819
13820 (void) SysCacheGetAttr(RELOID, tuple,
Anum_pg_class_relpartbound,
13821 &isnull);
13822 Assert(!isnull);

Thanks for the report. Attached a patch that adds a check that tuple is
valid before trying to dereference it.

Thanks,
Amit

On Mon, Nov 27, 2017 at 07:21:32PM +0900, Amit Langote wrote:

On 2017/11/27 18:13, bianpan2016@163.com wrote:

The following bug has been logged on the website:

Bug reference: 14928
Logged by: Pan Bian
Email address: bianpan2016@163.com
PostgreSQL version: 10.1
Operating system: Linux
Description:

Thanks for the report. Attached a patch that adds a check that tuple is
valid before trying to dereference it.

Thanks,
Amit

Got it.

Thanks a lot,
Pan Bian

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

On 2017/11/27 18:13, bianpan2016@163.com wrote:

Function SearchSysCacheCopy1() may return a NULL pointer if there is no
enough memory. But in function ATExecDetachPartition(), its return value is
not checked, which may result in NULL dereference (see line 13818).

Thanks for the report. Attached a patch that adds a check that tuple is
valid before trying to dereference it.

Pushed. Checking other SearchSysCache calls in these two files, I noted
a third instance of the same problem in StorePartitionKey(). However,
looking closer, StorePartitionKey never does anything at all with the
result of its SearchSysCache1(PARTRELID, ...) lookup, including never
releasing the syscache reference. How is it that we don't get refcount
leak warnings? I guess that must prove that that lookup always fails,
which is not too surprising since it seems to be against the partition
key info that we haven't stored yet. Anyway, I just diked that one
out, since it's clearly useless.

regards, tom lane

On 2017/11/28 9:25, Tom Lane wrote:

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

On 2017/11/27 18:13, bianpan2016@163.com wrote:

Function SearchSysCacheCopy1() may return a NULL pointer if there is no
enough memory. But in function ATExecDetachPartition(), its return value is
not checked, which may result in NULL dereference (see line 13818).

Thanks for the report. Attached a patch that adds a check that tuple is
valid before trying to dereference it.

Pushed.

Thank you.

Checking other SearchSysCache calls in these two files, I noted
a third instance of the same problem in StorePartitionKey(). However,
looking closer, StorePartitionKey never does anything at all with the
result of its SearchSysCache1(PARTRELID, ...) lookup, including never
releasing the syscache reference. How is it that we don't get refcount
leak warnings? I guess that must prove that that lookup always fails,
which is not too surprising since it seems to be against the partition
key info that we haven't stored yet. Anyway, I just diked that one
out, since it's clearly useless.

Thank you, too. I guess that SearchSysCache was from one of the earliest
versions of the patch, whereby we'd error out if the tuple we got out was
valid; that is, error for trying to set the partition of key of a table
that already had one.

Thanks,
Amit