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

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, but mostly
because it makes the code more fragile. Put a "goto fail" in the
wrong place, and kaboom!

We could do something like initialize all these variables to NULL
at the top and then write

if (values)
pfree(values);

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.)

regards, tom lane