Use of uninitialized variables in ExecFindPartition() for parent partition without leaves (HEAD only)

Hi Michael.

On 2017/11/30 9:07, Michael Paquier wrote:

Hi all,

Since commit 4e5fe9ad (committer Robert Haas and author Amit Langote),
coverity has been complaining that the new code of ExecFindPartition()
may use a set of values and isnull values which never get initialized.
This is a state which can be easily reached with the following SQLs of
a parent partition with no children:
create table parent_tab (a int) partition by list ((a+0));
insert into parent_tab values (1);

The mistake is visibly that FormPartitionKeyDatum() should be called
even for a root partition, initializing the fields of isnull and
values on the way. That's actually what happens in execMain.c's
ExecFindPartition() if you look at REL_10_STABLE. So the answer would
be to move a bit down the quick exit code. Attached is a patch.

Amit, that's your code. What do you think?

Thanks for the report. That's clearly a bug. :-(

Your patch seems enough to fix it. I added a test and expanded the
comment a bit in the attached updated version.

Thanks,
Amit

On 2017/11/30 10:11, Amit Langote wrote:

in the attached updated version.

Oops, I messed up taking the diff and mistakenly added noise to the patch.
Fixed in the attached.

Thanks,
Amit

On Thu, Nov 30, 2017 at 10:17 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Oops, I messed up taking the diff and mistakenly added noise to the patch.

Which is that bit:
- * BuildSlotPartitionKeyDescription
+ * ExecBuildSlotPartitionKeyDescription

Fixed in the attached.

For information, it is easy enough to run into rather nasty behaviors
here. Taking for example the test case in the last patch, but without
the actual fix:
-- no partitions, so fail
insert into range_parted values ('a', 11);
! ERROR: invalid memory alloc request size 2139062147
So the test case you are adding is a good thing to have, and I am fine
with the comment.

I have added a CF entry for this thread by the way
(https://commitfest.postgresql.org/16/1392/), and marked the thing as
ready for committer as we agree about the fix. Let's track properly
this issue until it gets committed.
--
Michael

On 2017/12/01 11:48, Michael Paquier wrote:

On Thu, Nov 30, 2017 at 10:17 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Oops, I messed up taking the diff and mistakenly added noise to the patch.

Which is that bit:
- * BuildSlotPartitionKeyDescription
+ * ExecBuildSlotPartitionKeyDescription

Yep.

Fixed in the attached.

For information, it is easy enough to run into rather nasty behaviors
here. Taking for example the test case in the last patch, but without
the actual fix:
-- no partitions, so fail
insert into range_parted values ('a', 11);
! ERROR: invalid memory alloc request size 2139062147

Yikes.

So the test case you are adding is a good thing to have, and I am fine
with the comment.

Thanks.

I have added a CF entry for this thread by the way
(https://commitfest.postgresql.org/16/1392/), and marked the thing as
ready for committer as we agree about the fix. Let's track properly
this issue until it gets committed.

Yeah, thanks.

Regards,
Amit

On Thu, Nov 30, 2017 at 11:54 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I have added a CF entry for this thread by the way
(https://commitfest.postgresql.org/16/1392/), and marked the thing as
ready for committer as we agree about the fix. Let's track properly
this issue until it gets committed.

Yeah, thanks.

Committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company