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

Started by Michael Paquierabout 8 years ago6 messages

michael.paquier@gmail.com

about 8 years ago

1 attachment(s)

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,
--
Michael

Attachments:

exec-partition-quickexit.patchapplication/octet-stream; name=exec-partition-quickexit.patchDownload

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index d275cefe1d..98d6d98d65 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -206,13 +206,6 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
 			slot = myslot;
 		}
 
-		/* Quick exit */
-		if (partdesc->nparts == 0)
-		{
-			result = -1;
-			break;
-		}
-
 		/*
 		 * Extract partition key from tuple. Expression evaluation machinery
 		 * that FormPartitionKeyDatum() invokes expects ecxt_scantuple to
@@ -223,6 +216,14 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
 		 */
 		ecxt->ecxt_scantuple = slot;
 		FormPartitionKeyDatum(parent, slot, estate, values, isnull);
+
+		/* Quick exit */
+		if (partdesc->nparts == 0)
+		{
+			result = -1;
+			break;
+		}
+
 		cur_index = get_partition_for_tuple(rel, values, isnull);
 
 		/*

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

about 8 years ago

In reply to: Michael Paquier (#1)

1 attachment(s)

Re: 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

Attachments:

exec-partition-quickexit-v2.patchtext/plain; charset=UTF-8; name=exec-partition-quickexit-v2.patchDownload

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index d275cefe1d..8b334ea2ff 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -206,13 +206,6 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
 			slot = myslot;
 		}
 
-		/* Quick exit */
-		if (partdesc->nparts == 0)
-		{
-			result = -1;
-			break;
-		}
-
 		/*
 		 * Extract partition key from tuple. Expression evaluation machinery
 		 * that FormPartitionKeyDatum() invokes expects ecxt_scantuple to
@@ -223,6 +216,17 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
 		 */
 		ecxt->ecxt_scantuple = slot;
 		FormPartitionKeyDatum(parent, slot, estate, values, isnull);
+
+		/*
+		 * Nothing for get_partition_for_tuple() to do if there are no
+		 * partitions to begin with.
+		 */
+		if (partdesc->nparts == 0)
+		{
+			result = -1;
+			break;
+		}
+
 		cur_index = get_partition_for_tuple(rel, values, isnull);
 
 		/*
@@ -472,7 +476,7 @@ FormPartitionKeyDatum(PartitionDispatch pd,
 }
 
 /*
- * BuildSlotPartitionKeyDescription
+ * ExecBuildSlotPartitionKeyDescription
  *
  * This works very much like BuildIndexValueDescription() and is currently
  * used for building error messages when ExecFindPartition() fails to find
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index b7b37dbc39..dcbaad8e2f 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -165,6 +165,10 @@ create table range_parted (
 	a text,
 	b int
 ) partition by range (a, (b+0));
+-- no partitions, so fail
+insert into range_parted values ('a', 11);
+ERROR:  no partition of relation "range_parted" found for row
+DETAIL:  Partition key of the failing row contains (a, (b + 0)) = (a, 11).
 create table part1 partition of range_parted for values from ('a', 1) to ('a', 10);
 create table part2 partition of range_parted for values from ('a', 10) to ('a', 20);
 create table part3 partition of range_parted for values from ('b', 1) to ('b', 10);
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 310b818076..0150b6bb0f 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -90,6 +90,10 @@ create table range_parted (
 	a text,
 	b int
 ) partition by range (a, (b+0));
+
+-- no partitions, so fail
+insert into range_parted values ('a', 11);
+
 create table part1 partition of range_parted for values from ('a', 1) to ('a', 10);
 create table part2 partition of range_parted for values from ('a', 10) to ('a', 20);
 create table part3 partition of range_parted for values from ('b', 1) to ('b', 10);

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

about 8 years ago

In reply to: Amit Langote (#2)

1 attachment(s)

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

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

Attachments:

exec-partition-quickexit-v3.patchtext/plain; charset=UTF-8; name=exec-partition-quickexit-v3.patchDownload

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 2fc411a9b5..8b334ea2ff 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -206,13 +206,6 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
 			slot = myslot;
 		}
 
-		/* Quick exit */
-		if (partdesc->nparts == 0)
-		{
-			result = -1;
-			break;
-		}
-
 		/*
 		 * Extract partition key from tuple. Expression evaluation machinery
 		 * that FormPartitionKeyDatum() invokes expects ecxt_scantuple to
@@ -223,6 +216,17 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
 		 */
 		ecxt->ecxt_scantuple = slot;
 		FormPartitionKeyDatum(parent, slot, estate, values, isnull);
+
+		/*
+		 * Nothing for get_partition_for_tuple() to do if there are no
+		 * partitions to begin with.
+		 */
+		if (partdesc->nparts == 0)
+		{
+			result = -1;
+			break;
+		}
+
 		cur_index = get_partition_for_tuple(rel, values, isnull);
 
 		/*
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index b7b37dbc39..dcbaad8e2f 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -165,6 +165,10 @@ create table range_parted (
 	a text,
 	b int
 ) partition by range (a, (b+0));
+-- no partitions, so fail
+insert into range_parted values ('a', 11);
+ERROR:  no partition of relation "range_parted" found for row
+DETAIL:  Partition key of the failing row contains (a, (b + 0)) = (a, 11).
 create table part1 partition of range_parted for values from ('a', 1) to ('a', 10);
 create table part2 partition of range_parted for values from ('a', 10) to ('a', 20);
 create table part3 partition of range_parted for values from ('b', 1) to ('b', 10);
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 310b818076..0150b6bb0f 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -90,6 +90,10 @@ create table range_parted (
 	a text,
 	b int
 ) partition by range (a, (b+0));
+
+-- no partitions, so fail
+insert into range_parted values ('a', 11);
+
 create table part1 partition of range_parted for values from ('a', 1) to ('a', 10);
 create table part2 partition of range_parted for values from ('a', 10) to ('a', 20);
 create table part3 partition of range_parted for values from ('b', 1) to ('b', 10);

Michael Paquier

michael.paquier@gmail.com

about 8 years ago

In reply to: Amit Langote (#3)

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

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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

about 8 years ago

In reply to: Michael Paquier (#4)

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

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

Robert Haas

robertmhaas@gmail.com

about 8 years ago

In reply to: Amit Langote (#5)

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

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