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
onpointer 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
returnedfrom 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
