Thread: Re: Small memory fixes for pg_createsubcriber
Andres Freund <andres@anarazel.de> writes: > 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". [...] > 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? Does it need a -D flag, wouldn't __has_attribute(malloc) (with the fallback definition in c.h) be enough? - ilmari
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Andres Freund <andres@anarazel.de> writes:
>> 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?
> Does it need a -D flag, wouldn't __has_attribute(malloc) (with the
> fallback definition in c.h) be enough?
libpq-fe.h has to be compilable by application code that has never
heard of pg_config.h let alone c.h, so we'd have to tread carefully
about not breaking that property. But it seems like this would be
worth looking into.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes: >> Andres Freund <andres@anarazel.de> writes: >>> 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? > >> Does it need a -D flag, wouldn't __has_attribute(malloc) (with the >> fallback definition in c.h) be enough? > > libpq-fe.h has to be compilable by application code that has never > heard of pg_config.h let alone c.h, so we'd have to tread carefully > about not breaking that property. But it seems like this would be > worth looking into. The fallback code isn't exactly complicated, so we could just duplicate it in libpq-fe.h: #ifndef __has_attribute #define __has_attribute(attribute) 0 #endif And then do something like this: #if __has_attribute (malloc) #define pg_attribute_malloc __attribute__((malloc)) #define pg_attribute_deallocator(...) __attribute__((malloc(__VA_ARGS__))) #else #define pg_attribute_malloc #define pg_attribute_deallocator(...) #endif > regards, tom lane - ilmari
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> libpq-fe.h has to be compilable by application code that has never
>> heard of pg_config.h let alone c.h, so we'd have to tread carefully
>> about not breaking that property. But it seems like this would be
>> worth looking into.
> The fallback code isn't exactly complicated, so we could just duplicate
> it in libpq-fe.h:
> #ifndef __has_attribute
> #define __has_attribute(attribute) 0
> #endif
The problem with that approach is the likelihood of stomping on
symbols that a calling application will use later. I think we
really need a controlling #ifdef check on some PG_FOO symbol
that we can be sure no outside application will have defined.
Roughly speaking,
#ifdef PG_USE_ALLOCATOR_CHECKS
#define pg_attribute_malloc __attribute__((malloc))
...
#else
#define pg_attribute_malloc
...
#endif
and then we could make definition of PG_USE_ALLOCATOR_CHECKS
be conditional on having the right compiler behavior, rather
than trusting that a nest of #ifdef checks is sufficient to
detect that.
regards, tom lane
I experimented with the other approach: hack libpq.so to depend on
dmalloc, leaving the rest of the system alone, so that libpq's
allocations could not be freed elsewhere nor vice versa.
It could not even get through initdb, crashing here:
replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
bool mark_as_comment)
{
PQExpBuffer newline = createPQExpBuffer();
...
free(newline); /* but don't free newline->data */
which upon investigation is expected, because createPQExpBuffer is
exported by libpq and therefore is returning space allocated within
the shlib. Diking out that particular free() call just allows it
to crash a bit later on, because initdb is totally full of direct
manipulations of PQExpBuffer contents.
This indicates to me that we have a *far* larger problem than
we thought, if we'd like to be totally clean on this point.
Realistically, it's not going to happen that way; it's just
not worth the trouble and notational mess. I think if we're
honest we should just document that such-and-such combinations
of libpq and our client programs will work on Windows.
For amusement's sake, totally dirty hack-and-slash patch attached.
(I tested this on macOS, with dmalloc from MacPorts; adjust SHLIB_LINK
to suit on other platforms.)
regards, tom lane
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 701810a272a..91dc2e14d99 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -81,7 +81,7 @@ endif
# that are built correctly for use in a shlib.
SHLIB_LINK_INTERNAL = -lpgcommon_shlib -lpgport_shlib
ifneq ($(PORTNAME), win32)
-SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket
-lnsl-lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS)
+SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket
-lnsl-lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) -L/opt/local/lib -ldmalloc $(PTHREAD_LIBS)
else
SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi32 -lssl -lsocket -lnsl -lresolv
-lintl-lm $(PTHREAD_LIBS), $(LIBS)) $(LDAP_LIBS_FE)
endif
@@ -116,11 +116,6 @@ backend_src = $(top_srcdir)/src/backend
# coding rule.
libpq-refs-stamp: $(shlib)
ifneq ($(enable_coverage), yes)
-ifeq (,$(filter solaris,$(PORTNAME)))
- @if nm -A -u $< 2>/dev/null | grep -v -e __cxa_atexit -e __tsan_func_exit | grep exit; then \
- echo 'libpq must not be calling any function which invokes exit'; exit 1; \
- fi
-endif
endif
touch $@
diff --git a/src/interfaces/libpq/fe-auth-sasl.h b/src/interfaces/libpq/fe-auth-sasl.h
index f06f547c07d..1abafe8d34b 100644
--- a/src/interfaces/libpq/fe-auth-sasl.h
+++ b/src/interfaces/libpq/fe-auth-sasl.h
@@ -146,7 +146,7 @@ typedef struct pg_fe_sasl_mech
* state: The opaque mechanism state returned by init()
*--------
*/
- void (*free) (void *state);
+ void (*saslfree) (void *state);
} pg_fe_sasl_mech;
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 85d1ca2864f..56ff7bc3b9d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -573,7 +573,7 @@ pqDropConnection(PGconn *conn, bool flushInput)
#endif
if (conn->sasl_state)
{
- conn->sasl->free(conn->sasl_state);
+ conn->sasl->saslfree(conn->sasl_state);
conn->sasl_state = NULL;
}
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 2546f9f8a50..7296d00b34f 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -20,6 +20,8 @@
#ifndef LIBPQ_INT_H
#define LIBPQ_INT_H
+#include <dmalloc.h>
+
/* We assume libpq-fe.h has already been included. */
#include "libpq-events.h"
Tom Lane <tgl@sss.pgh.pa.us> writes: > =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes: >> Tom Lane <tgl@sss.pgh.pa.us> writes: >>> libpq-fe.h has to be compilable by application code that has never >>> heard of pg_config.h let alone c.h, so we'd have to tread carefully >>> about not breaking that property. But it seems like this would be >>> worth looking into. > >> The fallback code isn't exactly complicated, so we could just duplicate >> it in libpq-fe.h: > >> #ifndef __has_attribute >> #define __has_attribute(attribute) 0 >> #endif > > The problem with that approach is the likelihood of stomping on > symbols that a calling application will use later. I think we > really need a controlling #ifdef check on some PG_FOO symbol > that we can be sure no outside application will have defined. > Roughly speaking, > > #ifdef PG_USE_ALLOCATOR_CHECKS As long as we're not checking for too many attributes, we could avoid introducing the __has_attribute symbol by doing: #if defined(__has_attribute) && __has_attribute(malloc) > #define pg_attribute_malloc __attribute__((malloc)) > ... > #else > #define pg_attribute_malloc > ... > #endif > > and then we could make definition of PG_USE_ALLOCATOR_CHECKS > be conditional on having the right compiler behavior, rather > than trusting that a nest of #ifdef checks is sufficient to > detect that. But I just looked at the clang attribute docs (https://clang.llvm.org/docs/AttributeReference.html#malloc), and it only has the argumentless version, not the one that that declares the matching deallocator, so you're right we need a more comprehensive check. > regards, tom lane - ilmari