Thread: Re: Small memory fixes for pg_createsubcriber
On Mon, Feb 10, 2025, at 1:31 PM, Ranier Vilela wrote:
Coverity has some reports about pg_createsubcriber.CID 1591322: (#1 of 1): Resource leak (RESOURCE_LEAK)10. leaked_storage: Variable dbname going out of scope leaks the storage it points to.Additionally there are several calls that are out of the ordinary, according to the Postgres API.According to the documentation:The correct function to free memory when using PQescapeLiteral and PQescapeIdentifier would be PQfreemem.
Hi,
From src/common/fe_memutils.c:
void
pg_free(void *ptr)
{
free(ptr);
}
From src/interfaces/libpq/fe-exec.c:
void
PQfreemem(void *ptr)
{
free(ptr);
}
There is no bug. They are the same behind the scenes. I'm fine changing it. It
is a new code and it wouldn't cause a lot of pain to backpatch patches in the
future.
@@ -1130,6 +1130,7 @@ check_and_drop_existing_subscriptions(PGconn *conn,
PQclear(res);
destroyPQExpBuffer(query);
+ PQfreemem(dbname);
}
Even if the pg_createsubscriber aims to run in a small amount of time, hence,
it is fine to leak memory, the initial commit cleaned up all variables but a
subsequent commit 4867f8a555c apparently didn't. Although it is just a low
impact improvement, it is better to be strict and shut up SASTs.
Em qua., 12 de fev. de 2025 às 00:54, Michael Paquier <michael@paquier.xyz> escreveu:
On Tue, Feb 11, 2025 at 01:32:32PM -0300, Euler Taveira wrote:
> There is no bug. They are the same behind the scenes. I'm fine changing it. It
> is a new code and it wouldn't cause a lot of pain to backpatch patches in the
> future.
On consistency grounds, and as this is documented in fe-exec.c at the
top of PQfreemem(), I can get behind the switch.
> Even if the pg_createsubscriber aims to run in a small amount of time, hence,
> it is fine to leak memory, the initial commit cleaned up all variables but a
> subsequent commit 4867f8a555c apparently didn't. Although it is just a low
> impact improvement, it is better to be strict and shut up SASTs.
check_and_drop_existing_subscriptions() is called once per database in
setup_subscriber(), and we are not going to have millions of them in
this list. We don't usually care for such short-lived things, but as
the original commit did the effort in d44032d01463, I don't see why we
cannot do it here, either.
Thanks Michael.
best regards,
Ranier Vilela
Hi, On 2025-02-11 13:32:32 -0300, Euler Taveira wrote: > On Mon, Feb 10, 2025, at 1:31 PM, Ranier Vilela wrote: > > Coverity has some reports about pg_createsubcriber. > > > > CID 1591322: (#1 of 1): Resource leak (RESOURCE_LEAK) > > 10. leaked_storage: Variable dbname going out of scope leaks the storage it points to. > > Additionally there are several calls that are out of the ordinary, according to the Postgres API. > > > > According to the documentation: > > libpq-exec <https://www.postgresql.org/docs/current/libpq-exec.html> > > > > > > The correct function to free memory when using PQescapeLiteral and PQescapeIdentifier would be PQfreemem. > > > > Hi, > > From src/common/fe_memutils.c: > > void > pg_free(void *ptr) > { > free(ptr); > } > > From src/interfaces/libpq/fe-exec.c: > > void > PQfreemem(void *ptr) > { > free(ptr); > } > > There is no bug. They are the same behind the scenes. I'm fine changing it. It > is a new code and it wouldn't cause a lot of pain to backpatch patches in the > future. That *is* a bug. On windows the allocator that a shared library (i.e. libpq) uses, may *not* be the same as the one that an executable (i.e. pg_createsubscriber). It's not correct to free memory allocated in a shared library just with free, it has to go through the library's free. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2025-02-11 13:32:32 -0300, Euler Taveira wrote:
>> There is no bug. They are the same behind the scenes.
> That *is* a bug. On windows the allocator that a shared library (i.e. libpq)
> uses, may *not* be the same as the one that an executable
> (i.e. pg_createsubscriber). It's not correct to free memory allocated in a
> shared library just with free, it has to go through the library's free.
Indeed. This is particularly pernicious because it will work even on
Windows under common scenarios (which no doubt explains the lack of
field reports). From the last time we discussed this [1]:
It seems to work fine as long as a debug-readline is paired with a
debug-psql or a release-readline is paired with a release-psql.
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.
regards, tom lane
[1] https://www.postgresql.org/message-id/20240709225934.746y5fg3kgxkyant@awork3.anarazel.de
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
Michael Paquier <michael@paquier.xyz> writes:
> I have looked at bit at the uses of PQescapeLiteral() and
> PQescapeIdentifier() in the tree. On top of the one in pg_amcheck you
> are just pointing to, there is an inconsistency in pg_upgrade.c for
> set_locale_and_encoding() where datlocale_literal may be allocated
> with a pg_strdup() or a PQescapeLiteral() depending on the path. The
> code has been using PQfreemem() for the pg_strdup() allocation, which
> is logically incorrect.
Yeah, I suspected there would be places like that. It just hasn't
mattered in practice up to now. (I have a vague recollection that
Windows used to be pickier about this, but evidently not in recent
years.)
I spent a little time earlier today seeing what I could do with the
use-dmalloc patch I posted earlier. It turns out you can get through
initdb after s/free/PQfreemem/ in just two places, and then the
backend works fine. But psql is a frickin' disaster --- there's
free's of strings made with PQExpBuffer all over its backslash-command
handling, and no easy way to clean it up. Maybe other clients will
be less of a mess, but I'm not betting on that.
regards, tom lane