Re: Fix a resource leak (src/backend/utils/adt/rowtypes.c)

Em seg., 14 de abr. de 2025 às 18:11, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:

> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Sun, Apr 13, 2025 at 7:34 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
> wrote:
> >> The function *record_in* has a new report about resource leak.
> >> I think Coverity is right.
>
> > I agree, for small values of "right". The memory isn't formally leaked
> > because it will be eventually released when the containing memory
> > context is deleted, but it's unclear why we should bother to clean up
> > the memory in the normal path yet skip it here. I wondered whether the
> > existing pfree calls were added in response to some specific
> > complaint, but it doesn't appear so: they date back to Tom's 2004-era
> > commit a3704d3deca6d08013a6b1db0432b75dc6b78d28, the commit message
> > for which is rather more brief than what is typical today. Still, it
> > seems safer to bet on the pfree being a good idea than on the reverse,
> > because record_in() can be called lots of times in a single
> > transaction.
>
> Well, the pfree's in the main path do, but the "fail:" code path is
> considerably more recent (ccff2d20e). In the latter, I believe I
> thought it wasn't worth the bookkeeping to keep track of whether these
> allocations had been made yet. Note the comment
>
> /* exit here once we've done lookup_rowtype_tupdesc */
>
> i.e. there is (was) exactly one criterion for whether to "goto fail"
> or just return. I don't love the proposed patch, partly because it
> doesn't bother to update that comment after falsifying it,

I can't see how the patch can do this.
The comment is still valid, isn't it?
But it's incomplete from the beginning, it could be:
/*exit here once we've done lookup_rowtype_tupdesc or when fail */

but mostly
> because it makes the code more fragile. Put a "goto fail" in the
> wrong place, and kaboom!
>
The only fragile thing is the StringInfoData buf.
That's why the patch moves the initialization of buf, to avoid the "kaboom".

>
> We could do something like initialize all these variables to NULL
> at the top and then write
>
> if (values)
> pfree(values);
>
This is not necessary, both in the normal path and in the failure path,
all variables are correctly initialized and released correctly too.

> et cetera. But I don't see the point. Frankly, if I wanted to make
> this more consistent I would delete the pfrees in the main path.
> Exactly zero evidence has been offered that that would create a leak
> of significance, and the existing code goes against our general advice
> that retail pfree's are more expensive than letting context reset
> clean up. (The fact that Coverity doesn't understand that doesn't
> make it not true.)
>
I think something needs to be done, and I vote to free the memory when it
fails.
The function as it is, has "bipolar disorder".
Sometimes they behave one way, other times they behave another.
IMO, there is also zero evidence that not releasing it would be beneficial.

best regards,
Ranier Vilela