Thread: [MASSMAIL]Coverity complains about simplehash.h's SH_STAT()

Today's Coverity run produced this:

/srv/coverity/git/pgsql-git/postgresql/src/include/lib/simplehash.h: 1138 in bh_nodeidx_stat()
1132             avg_collisions = 0;
1133         }
1134
1135         sh_log("size: " UINT64_FORMAT ", members: %u, filled: %f, total chain: %u, max chain: %u, avg chain: %f,
total_collisions:%u, max_collisions: %u, avg_collisions: %f", 
1136                tb->size, tb->members, fillfactor, total_chain_length, max_chain_length, avg_chain_length,
1137                total_collisions, max_collisions, avg_collisions);
>>>     CID 1596268:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "collisions" going out of scope leaks the storage it points to.
1138     }
1139
1140     #endif                            /* SH_DEFINE */

I have no idea why we didn't see this warning before --- but AFAICS
it's quite right, and it looks like a nontrivial amount of memory
could be at stake:

    uint32       *collisions = (uint32 *) palloc0(tb->size * sizeof(uint32));

I realize this function is only debug support, but wouldn't it
be appropriate to pfree(collisions) before exiting?

            regards, tom lane

Hi,

On 2024-04-07 21:03:53 -0400, Tom Lane wrote:
> Today's Coverity run produced this:
> 
> /srv/coverity/git/pgsql-git/postgresql/src/include/lib/simplehash.h: 1138 in bh_nodeidx_stat()
> 1132             avg_collisions = 0;
> 1133         }
> 1134     
> 1135         sh_log("size: " UINT64_FORMAT ", members: %u, filled: %f, total chain: %u, max chain: %u, avg chain: %f,
total_collisions:%u, max_collisions: %u, avg_collisions: %f",
 
> 1136                tb->size, tb->members, fillfactor, total_chain_length, max_chain_length, avg_chain_length,
> 1137                total_collisions, max_collisions, avg_collisions);
> >>>     CID 1596268:  Resource leaks  (RESOURCE_LEAK)
> >>>     Variable "collisions" going out of scope leaks the storage it points to.
> 1138     }
> 1139     
> 1140     #endif                            /* SH_DEFINE */
> 
> I have no idea why we didn't see this warning before --- but AFAICS
> it's quite right, and it looks like a nontrivial amount of memory
> could be at stake:
> 
>     uint32       *collisions = (uint32 *) palloc0(tb->size * sizeof(uint32));
> 
> I realize this function is only debug support, but wouldn't it
> be appropriate to pfree(collisions) before exiting?

It indeed looks like that memory should be freed. Very odd that coverity
started to complain about that just now.  If coverity had started to complain
after da41d71070d, I'd understand, but that was ~5 years ago.

I can't see a way it could hurt in the back branches, so I'm inclined to
backpatch the pfree?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2024-04-07 21:03:53 -0400, Tom Lane wrote:
>> I realize this function is only debug support, but wouldn't it
>> be appropriate to pfree(collisions) before exiting?

> It indeed looks like that memory should be freed. Very odd that coverity
> started to complain about that just now.  If coverity had started to complain
> after da41d71070d, I'd understand, but that was ~5 years ago.

If we recently added a new simplehash caller, Coverity might see that
as a new bug.  Still doesn't explain why nothing about the old callers.
We might have over-hastily dismissed such warnings as uninteresting,
perhaps.

> I can't see a way it could hurt in the back branches, so I'm inclined to
> backpatch the pfree?

+1

            regards, tom lane

On 2024-04-07 21:41:23 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I can't see a way it could hurt in the back branches, so I'm inclined to
> > backpatch the pfree?
> 
> +1

Done