Coverity complains about simplehash.h's SH_STAT()

  • Jump to comment-1
    tgl@sss.pgh.pa.us2024-04-08T01:04:00+00:00
    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
    • Jump to comment-1
      andres@anarazel.de2024-04-08T01:31:52+00:00
      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
      • Jump to comment-1
        tgl@sss.pgh.pa.us2024-04-08T01:41:29+00:00
        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
        • Jump to comment-1
          andres@anarazel.de2024-04-08T02:10:24+00:00
          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