Обсуждение: 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.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
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