Re: BUG #14928: Unchecked SearchSysCacheCopy1() return value

On 2017/11/28 9:25, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
>> On 2017/11/27 18:13, bianpan2016(at)163(dot)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