NULL checks of deferenced pointers in picksplit method of intarray

Started by Michael Paquieralmost 11 years ago3 messages

michael.paquier@gmail.com

almost 11 years ago

1 attachment(s)

Hi all,

Coverity is pointing out that _int_split.c has unnecessary checks for
deferenced pointers in 5 places.

-                       if (inter_d != (ArrayType *) NULL)
-                               pfree(inter_d);
+                       pfree(inter_d);
In this case inter_d is generated by inner_int_inter, a routine that
always generates an ArrayType with at least new_intArrayType.

In two places there is as well this pattern:
-                       if (datum_l)
-                               pfree(datum_l);
-                       if (union_dr)
-                               pfree(union_dr);
+                       pfree(datum_l);
+                       pfree(union_dr);
And that one:
-                       if (datum_r)
-                               pfree(datum_r);
-                       if (union_dl)
-                               pfree(union_dl);
+                       pfree(datum_r);
+                       pfree(union_dl);
union_dr and union_dl are generated by inner_int_union which never
returns NULL. Similarly, datum_r and datum_l are created with
copy_intArrayType the first time, which never returns NULL, and their
values are changed at each loop step. Also, as far as I understood
from this code, no elements manipulated are NULL, perhaps this is
worth an assertion?

Attached is a patch to adjust those things.
Regards,
--
Michael

Attachments:

20150130_intarray_fix_dereferences.patchtext/x-diff; charset=US-ASCII; name=20150130_intarray_fix_dereferences.patchDownload

diff --git a/contrib/intarray/_int_gist.c b/contrib/intarray/_int_gist.c
index 53abcc4..876a7b9 100644
--- a/contrib/intarray/_int_gist.c
+++ b/contrib/intarray/_int_gist.c
@@ -416,9 +416,7 @@ g_int_picksplit(PG_FUNCTION_ARGS)
 			size_waste = size_union - size_inter;
 
 			pfree(union_d);
-
-			if (inter_d != (ArrayType *) NULL)
-				pfree(inter_d);
+			pfree(inter_d);
 
 			/*
 			 * are these a more promising split that what we've already seen?
@@ -517,10 +515,8 @@ g_int_picksplit(PG_FUNCTION_ARGS)
 		/* pick which page to add it to */
 		if (size_alpha - size_l < size_beta - size_r + WISH_F(v->spl_nleft, v->spl_nright, 0.01))
 		{
-			if (datum_l)
-				pfree(datum_l);
-			if (union_dr)
-				pfree(union_dr);
+			pfree(datum_l);
+			pfree(union_dr);
 			datum_l = union_dl;
 			size_l = size_alpha;
 			*left++ = i;
@@ -528,10 +524,8 @@ g_int_picksplit(PG_FUNCTION_ARGS)
 		}
 		else
 		{
-			if (datum_r)
-				pfree(datum_r);
-			if (union_dl)
-				pfree(union_dl);
+			pfree(datum_r);
+			pfree(union_dl);
 			datum_r = union_dr;
 			size_r = size_beta;
 			*right++ = i;

Kevin Grittner

kgrittn@ymail.com

almost 11 years ago

In reply to: Michael Paquier (#1)

Re: NULL checks of deferenced pointers in picksplit method of intarray

Michael Paquier <michael.paquier@gmail.com> wrote:

Coverity is pointing out that _int_split.c has unnecessary checks
for deferenced pointers in 5 places.

Attached is a patch to adjust those things.

Pushed. Thanks!

Also, as far as I understood from this code, no elements
manipulated are NULL, perhaps this is worth an assertion?

I'm not clear where you were thinking of, but anyway that seemed
like a separate patch if we're going to do it, so I went ahead with
pushing the issued Coverity flagged. The arguments to the function
don't need such a check because the function is exposed to SQL with
the STRICT option (but you probably already knew that). While
reviewing the safety of this patch the only place that I ran across
that I felt maybe deserved an assertion was that n >= 0 near the
top of copy_intArrayType(), but that seems marginal.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

almost 11 years ago

In reply to: Kevin Grittner (#2)

Re: NULL checks of deferenced pointers in picksplit method of intarray

On Tue, Feb 17, 2015 at 6:49 AM, Kevin Grittner <kgrittn@ymail.com> wrote:

Michael Paquier <michael.paquier@gmail.com> wrote:

Coverity is pointing out that _int_split.c has unnecessary checks
for deferenced pointers in 5 places.

Attached is a patch to adjust those things.

Pushed. Thanks!

Thanks.

Also, as far as I understood from this code, no elements
manipulated are NULL, perhaps this is worth an assertion?

I'm not clear where you were thinking of, but anyway that seemed
like a separate patch if we're going to do it, so I went ahead with
pushing the issued Coverity flagged. The arguments to the function
don't need such a check because the function is exposed to SQL with
the STRICT option (but you probably already knew that). While
reviewing the safety of this patch the only place that I ran across
that I felt maybe deserved an assertion was that n >= 0 near the
top of copy_intArrayType(), but that seems marginal.

Yeah, we don't do that for the other STRICT functions, let's not do it then.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers