Hi, On 2025-02-12 11:02:04 -0500, Tom Lane wrote: > I wish we had some way to detect misuses automatically ... > > This seems like the sort of bug that Coverity could detect if only it > knew to look, but I have no idea if it could be configured that way. > Maybe some weird lashup with a debugging malloc library would be > another way.
Recent gcc actually has a fairly good way to detect this kind of issue: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute in particular, the variant of the attribute with "deallocator". I'd started on a patch to add those, but ran out of cycles for 18. I quickly checked out my branch and added the relevant attributes to PQescapeLiteral(), PQescapeIdentifier() and that indeed finds the issue pointed out in this thread: ../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_createsubscriber.c: In function 'create_logical_replication_slot': ../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_createsubscriber.c:1332:9: warning: 'pg_free' called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc] 1332 | pg_free(slot_name_esc); | ^~~~~~~~~~~~~~~~~~~~~~ ../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_createsubscriber.c:1326:25: note: returned from 'PQescapeLiteral' 1326 | slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ... but also finds one more: [62/214 42 28%] Compiling C object src/bin/pg_amcheck/pg_amcheck.p/pg_amcheck.c.o ../../../../../home/andres/src/postgresql/src/bin/pg_amcheck/pg_amcheck.c: In function 'main': ../../../../../home/andres/src/postgresql/src/bin/pg_amcheck/pg_amcheck.c:563:25: warning: 'pfree' called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc] 563 | pfree(schema); | ^~~~~~~~~~~~~ ../../../../../home/andres/src/postgresql/src/bin/pg_amcheck/pg_amcheck.c:556:34: note: returned from 'PQescapeIdentifier' 556 | schema = PQescapeIdentifier(conn, opts.install_schema, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 557 | strlen(opts.install_schema)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Note that that doesn't just require adding the attributes to PQescapeIdentifier() etc, but also to pg_malloc(), as the latter is how gcc knows that pg_pfree() is a deallocating function. Particularly for something like libpq it's not quitetrivial to add attributes like this, of course. We can't even depend on pg_config.h. One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's "armed" by a commandline -D flag, if the compiler is supported? Greetings, Andres Freund
