NULL checks of deferenced pointers in picksplit method of intarray

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

Michael Paquier <michael(dot)paquier(at)gmail(dot)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

On Tue, Feb 17, 2015 at 6:49 AM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)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