Re: Potential memory leak in contrib/intarray's g_intbig_compress

On Thu, 13 Jul 2023 at 17:20, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> writes:
> > My collegue Konstantin (cc-ed) noticed that the GiST code of intarray
> > may leak memory in certain index operations:
>
> Can you demonstrate an actual problem here, that is a query-lifespan leak?
>
> IMO, the coding rule in the GiST and GIN AMs is that the AM code is
> responsible for running all opclass-supplied functions in suitably
> short-lived memory contexts, so that leaks within those functions
> don't cause problems. This is different from btree which requires
> comparison functions to not leak. The rationale for having different
> conventions is that btree comparison functions are typically simple
> enough to be able to deal with such a restriction, whereas GiST
> and GIN opclasses are often complex critters for which it'd be too
> bug-prone to insist on leakproofness. So it seems worth the cost
> to make the AM itself set up a throwaway memory context.
>
> (I don't recall offhand about which rule the other AMs use.
> I'm also not sure where or if this choice is documented.)
>
> In the case at hand, I think the pfree is useless and was installed
> by somebody who had mis-extrapolated from btree rules. So what
> I'm thinking would be sufficient is to drop it altogether:
>
> - if (in != DatumGetArrayTypeP(entry->key))
> - pfree(in);

Looks like it's indeed a useless pfree call here - all paths that I
could find that lead to GiST's compress procedure are encapsulated in
a temporary context that is reset fairly quickly after use (at most
this memory context would live for the duration of the recursive
splitting of pages up the tree, but I haven't verified this
hypotheses).

There are similar pfree calls in the _int_gist.c file's g_int_compress
function, which made me think we do need to clean up after use, but
indeed these pfrees are useless (or even harmful if bug #17888 can be
trusted)

Kind regards,

Matthias van de Meent.