pgsql-hackers
❮
Coverity complains about simplehash.h's SH_STAT()
- Jump to comment-1Tom Lane<tgl@sss.pgh.pa.us>Apr 8, 2024, 1:04 AM UTCToday's Coverity run produced this:
/srv/coverity/git/pgsql-git/postgresql/src/include/lib/simplehash.h: 1138 in bhnodeidxstat()
1132 avg_collisions = 0;
1133 }
1134
1135 shlog("size: " UINT64FORMAT ", members: %u, filled: %f, total chain: %u, max chain: %u, avg chain: %f, totalcollisions: %u, maxcollisions: %u, avg_collisions: %f",
1136 tb->size, tb->members, fillfactor, totalchainlength, maxchainlength, avgchainlength,
1137 totalcollisions, maxcollisions, avg_collisions);CID 1596268: Resource leaks (RESOURCE_LEAK)
1138 }
Variable "collisions" going out of scope leaks the storage it points to.
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:
I realize this function is only debug support, but wouldn't ituint32 *collisions = (uint32 *) palloc0(tb->size * sizeof(uint32));
be appropriate to pfree(collisions) before exiting?regards, tom lane- Jump to comment-1Andres Freund<andres@anarazel.de>Apr 8, 2024, 1:31 AM UTCHi,
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 bhnodeidxstat()
1132 avg_collisions = 0;
1133 }
1134
1135 shlog("size: " UINT64FORMAT ", members: %u, filled: %f, total chain: %u, max chain: %u, avg chain: %f, totalcollisions: %u, maxcollisions: %u, avg_collisions: %f",1136 tb->size, tb->members, fillfactor, totalchainlength, maxchainlength, avgchainlength,
1137 totalcollisions, maxcollisions, 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));
It indeed looks like that memory should be freed. Very odd that coverity
I realize this function is only debug support, but wouldn't it
be appropriate to pfree(collisions) before exiting?
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- Jump to comment-1Tom Lane<tgl@sss.pgh.pa.us>Apr 8, 2024, 1:41 AM UTCAndres 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
If we recently added a new simplehash caller, Coverity might see that
started to complain about that just now. If coverity had started to complain
after da41d71070d, I'd understand, but that was ~5 years ago.
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
+1
backpatch the pfree?regards, tom lane- Jump to comment-1Andres Freund<andres@anarazel.de>Apr 8, 2024, 2:10 AM UTCOn 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?
Done
+1