Thread: Fix a resource leak (src/backend/utils/adt/rowtypes.c)
Hi.
Per Coverity.
CID 1608916: (#1 of 1): Resource leak (RESOURCE_LEAK)
52. leaked_storage: Variable buf going out of scope leaks the storage buf.data points to. The function *record_in* has a new report about resource leak.
I think Coverity is right.
The normal path of the function frees the memory of several variables used.
Therefore the failure path should also free them.
A quick search on the web shows several occurrences of "malformed record literal", therefore failure is common in this function.
Therefore the failure path should also free them.
A quick search on the web shows several occurrences of "malformed record literal", therefore failure is common in this function.
Although Coveriy reports the leak of only buf.data, the variables *values* and *nulls* should also be released.
While there, move the creation of stringdata, to ensure that in case of failure, the buf.data variable is released correctly.
Attached a path.
best regards,
Ranier Vilela
Attachment
On Sun, Apr 13, 2025 at 7:34 PM Ranier Vilela <ranier.vf@gmail.com> wrote: > CID 1608916: (#1 of 1): Resource leak (RESOURCE_LEAK) > 52. leaked_storage: Variable buf going out of scope leaks the storage buf.data points to. > > 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. -- Robert Haas EDB: http://www.enterprisedb.com
Em seg., 14 de abr. de 2025 às 16:59, Robert Haas <robertmhaas@gmail.com> escreveu:
On Sun, Apr 13, 2025 at 7:34 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> CID 1608916: (#1 of 1): Resource leak (RESOURCE_LEAK)
> 52. leaked_storage: Variable buf going out of scope leaks the storage buf.data points to.
>
> The function *record_in* has a new report about resource leak.
> I think Coverity is right.
I agree, for small values of "right".
Thanks.
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,
Thanks for researching.
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.
I think that material for v18, although there were no reported concerns.
best regards,
Ranier Vilela
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Apr 13, 2025 at 7:34 PM Ranier Vilela <ranier.vf@gmail.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
Em seg., 14 de abr. de 2025 às 18:11, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Apr 13, 2025 at 7:34 PM Ranier Vilela <ranier.vf@gmail.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 */
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