Detecting use-after-free bugs using gcc's malloc() attribute
Hi,
I played around with adding
__attribute__((malloc(free_func), malloc(another_free_func)))
annotations to a few functions in pg. See
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
Adding them to pg_list.h seems to have found two valid issues when compiling
without optimization:
[1001/2331 22 42%] Compiling C object src/backend/postgres_lib.a.p/commands_tablecmds.c.o
../../../../home/andres/src/postgresql/src/backend/commands/tablecmds.c: In function ‘ATExecAttachPartition’:
../../../../home/andres/src/postgresql/src/backend/commands/tablecmds.c:17966:25: warning: pointer ‘partBoundConstraint’ may be used after ‘list_concat’ [-Wuse-after-free]
17966 | get_proposed_default_constraint(partBoundConstraint);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../home/andres/src/postgresql/src/backend/commands/tablecmds.c:17919:26: note: call to ‘list_concat’ here
17919 | partConstraint = list_concat(partBoundConstraint,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
17920 | RelationGetPartitionQual(rel));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[1233/2331 22 52%] Compiling C object src/backend/postgres_lib.a.p/rewrite_rewriteHandler.c.o
../../../../home/andres/src/postgresql/src/backend/rewrite/rewriteHandler.c: In function ‘rewriteRuleAction’:
../../../../home/andres/src/postgresql/src/backend/rewrite/rewriteHandler.c:550:41: warning: pointer ‘newjointree’ may be used after ‘list_concat’ [-Wuse-after-free]
550 | checkExprHasSubLink((Node *) newjointree);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../home/andres/src/postgresql/src/backend/rewrite/rewriteHandler.c:542:33: note: call to ‘list_concat’ here
542 | list_concat(newjointree, sub_action->jointree->fromlist);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Both of these bugs seem somewhat older, the latter going back to 19ff959bff0,
in 2005. I'm a bit surprised that we haven't hit them before, via
DEBUG_LIST_MEMORY_USAGE?
When compiling with optimization, another issue is reported:
[508/1814 22 28%] Compiling C object src/backend/postgres_lib.a.p/commands_explain.c.o
../../../../home/andres/src/postgresql/src/backend/commands/explain.c: In function 'ExplainNode':
../../../../home/andres/src/postgresql/src/backend/commands/explain.c:2037:25: warning: pointer 'ancestors' may be used after 'lcons' [-Wuse-after-free]
2037 | show_upper_qual(plan->qual, "Filter", planstate, ancestors, es);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'show_group_keys',
inlined from 'ExplainNode' at ../../../../home/andres/src/postgresql/src/backend/commands/explain.c:2036:4:
../../../../home/andres/src/postgresql/src/backend/commands/explain.c:2564:21: note: call to 'lcons' here
2564 | ancestors = lcons(plan, ancestors);
| ^~~~~~~~~~~~~~~~~~~~~~
which looks like it might be valid - the caller's "ancestors" variable could
now be freed? There do appear to be further instances of the issue, e.g. in
show_agg_keys(), that aren't flagged for some reason.
For something like pg_list.h the malloc(free) attribute is a bit awkward to
use, because one a) needs to list ~30 functions that can free a list and b)
the referenced functions need to be declared.
In my quick hack I just duplicated the relevant part of pg_list.h and added
the appropriate attributes to the copy of the original declaration.
I also added such attributes to bitmapset.h and palloc() et al, but that
didn't find existing bugs. It does find use-after-free instances if I add
some, similarly it does find cases of mismatching palloc with free etc.
The attached prototype is quite rough and will likely fail on anything but a
recent gcc (likely >= 12).
Do others think this would be useful enough to be worth polishing? And do you
agree the warnings above are bugs?
Greetings,
Andres Freund
Attachments:
v1-0001-Add-allocator-attributes.patchtext/x-diff; charset=us-asciiDownload
From e05c1260ee70457e9a899d5c32e5b85702193739 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 26 Jun 2023 12:17:30 -0700
Subject: [PATCH v1 1/2] Add allocator attributes.
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/c.h | 9 ++++
src/include/nodes/bitmapset.h | 40 ++++++++++-----
src/include/nodes/pg_list.h | 97 +++++++++++++++++++++++++++++++++++
src/include/utils/palloc.h | 55 ++++++++++++--------
4 files changed, 167 insertions(+), 34 deletions(-)
diff --git a/src/include/c.h b/src/include/c.h
index f69d739be57..920fdf983a1 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -295,6 +295,15 @@
#define unlikely(x) ((x) != 0)
#endif
+#if defined(__GNUC__) && !defined(__clang__)
+#define pg_malloc_attr(deallocator) malloc(deallocator)
+#define pg_malloc_attr_i(deallocator, ptr_at) malloc(deallocator, ptr_at)
+#else
+#define pg_malloc_attr(deallocator)
+#define pg_malloc_attr_i(deallocator, ptr_at)
+#endif
+
+
/*
* CppAsString
* Convert the argument to a string, using the C preprocessor.
diff --git a/src/include/nodes/bitmapset.h b/src/include/nodes/bitmapset.h
index 14de6a9ff1e..5037f6907ec 100644
--- a/src/include/nodes/bitmapset.h
+++ b/src/include/nodes/bitmapset.h
@@ -78,15 +78,27 @@ typedef enum
* function prototypes in nodes/bitmapset.c
*/
-extern Bitmapset *bms_copy(const Bitmapset *a);
-extern bool bms_equal(const Bitmapset *a, const Bitmapset *b);
-extern int bms_compare(const Bitmapset *a, const Bitmapset *b);
-extern Bitmapset *bms_make_singleton(int x);
+extern Bitmapset *bms_add_member(Bitmapset *a, int x);
+extern Bitmapset *bms_del_member(Bitmapset *a, int x);
+extern Bitmapset *bms_add_members(Bitmapset *a, const Bitmapset *b);
+extern Bitmapset *bms_add_range(Bitmapset *a, int lower, int upper);
+extern Bitmapset *bms_int_members(Bitmapset *a, const Bitmapset *b);
+extern Bitmapset *bms_del_members(Bitmapset *a, const Bitmapset *b);
+extern Bitmapset *bms_join(Bitmapset *a, Bitmapset *b);
extern void bms_free(Bitmapset *a);
-extern Bitmapset *bms_union(const Bitmapset *a, const Bitmapset *b);
-extern Bitmapset *bms_intersect(const Bitmapset *a, const Bitmapset *b);
-extern Bitmapset *bms_difference(const Bitmapset *a, const Bitmapset *b);
+#define BMS_ALLOC_ATTRIBUTES __attribute__((pg_malloc_attr(bms_free), pg_malloc_attr(bms_add_member), pg_malloc_attr(bms_del_member), pg_malloc_attr(bms_add_members), pg_malloc_attr(bms_add_range), pg_malloc_attr(bms_int_members), pg_malloc_attr(bms_del_members), pg_malloc_attr(bms_join), warn_unused_result))
+
+extern Bitmapset *bms_copy(const Bitmapset *a) BMS_ALLOC_ATTRIBUTES;
+extern bool bms_equal(const Bitmapset *a, const Bitmapset *b);
+extern int bms_compare(const Bitmapset *a, const Bitmapset *b);
+extern Bitmapset *bms_make_singleton(int x) BMS_ALLOC_ATTRIBUTES;
+extern void bms_free(Bitmapset *a);
+
+extern Bitmapset *bms_union(const Bitmapset *a, const Bitmapset *b) BMS_ALLOC_ATTRIBUTES;
+extern Bitmapset *bms_intersect(const Bitmapset *a, const Bitmapset *b) BMS_ALLOC_ATTRIBUTES;
+extern Bitmapset *bms_difference(const Bitmapset *a, const Bitmapset *b) BMS_ALLOC_ATTRIBUTES;
+
extern bool bms_is_subset(const Bitmapset *a, const Bitmapset *b);
extern BMS_Comparison bms_subset_compare(const Bitmapset *a, const Bitmapset *b);
extern bool bms_is_member(int x, const Bitmapset *a);
@@ -106,13 +118,13 @@ extern BMS_Membership bms_membership(const Bitmapset *a);
/* these routines recycle (modify or free) their non-const inputs: */
-extern Bitmapset *bms_add_member(Bitmapset *a, int x);
-extern Bitmapset *bms_del_member(Bitmapset *a, int x);
-extern Bitmapset *bms_add_members(Bitmapset *a, const Bitmapset *b);
-extern Bitmapset *bms_add_range(Bitmapset *a, int lower, int upper);
-extern Bitmapset *bms_int_members(Bitmapset *a, const Bitmapset *b);
-extern Bitmapset *bms_del_members(Bitmapset *a, const Bitmapset *b);
-extern Bitmapset *bms_join(Bitmapset *a, Bitmapset *b);
+extern Bitmapset *bms_add_member(Bitmapset *a, int x) BMS_ALLOC_ATTRIBUTES;
+extern Bitmapset *bms_del_member(Bitmapset *a, int x) BMS_ALLOC_ATTRIBUTES;
+extern Bitmapset *bms_add_members(Bitmapset *a, const Bitmapset *b) BMS_ALLOC_ATTRIBUTES;
+extern Bitmapset *bms_add_range(Bitmapset *a, int lower, int upper) BMS_ALLOC_ATTRIBUTES;
+extern Bitmapset *bms_int_members(Bitmapset *a, const Bitmapset *b) BMS_ALLOC_ATTRIBUTES;
+extern Bitmapset *bms_del_members(Bitmapset *a, const Bitmapset *b) BMS_ALLOC_ATTRIBUTES;
+extern Bitmapset *bms_join(Bitmapset *a, Bitmapset *b) BMS_ALLOC_ATTRIBUTES;
/* support for iterating through the integer elements of a set: */
extern int bms_next_member(const Bitmapset *a, int prevbit);
diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h
index 529a382d284..285d8c2c7ed 100644
--- a/src/include/nodes/pg_list.h
+++ b/src/include/nodes/pg_list.h
@@ -632,4 +632,101 @@ extern void list_sort(List *list, list_sort_comparator cmp);
extern int list_int_cmp(const ListCell *p1, const ListCell *p2);
extern int list_oid_cmp(const ListCell *p1, const ListCell *p2);
+
+#define PG_LIST_ALLOC_ATTR __attribute__(( \
+ pg_malloc_attr(lappend), \
+ pg_malloc_attr(lappend_int), \
+ pg_malloc_attr(lappend_oid), \
+ pg_malloc_attr(lappend_xid), \
+ pg_malloc_attr(list_insert_nth), \
+ pg_malloc_attr(list_insert_nth_int), \
+ pg_malloc_attr(list_insert_nth_oid), \
+ pg_malloc_attr_i(lcons, 2), \
+ pg_malloc_attr_i(lcons_int, 2), \
+ pg_malloc_attr_i(lcons_oid, 2), \
+ pg_malloc_attr(list_concat), \
+ pg_malloc_attr(list_truncate), \
+ pg_malloc_attr(list_delete), \
+ pg_malloc_attr(list_delete_ptr), \
+ pg_malloc_attr(list_delete_int), \
+ pg_malloc_attr(list_delete_oid), \
+ pg_malloc_attr(list_delete_first), \
+ pg_malloc_attr(list_delete_last), \
+ pg_malloc_attr(list_delete_first_n), \
+ pg_malloc_attr(list_delete_nth_cell), \
+ pg_malloc_attr(list_delete_cell), \
+ pg_malloc_attr(list_union), \
+ pg_malloc_attr(list_append_unique), \
+ pg_malloc_attr(list_append_unique_ptr), \
+ pg_malloc_attr(list_append_unique_int), \
+ pg_malloc_attr(list_append_unique_oid), \
+ pg_malloc_attr(list_concat_unique), \
+ pg_malloc_attr(list_concat_unique_ptr), \
+ pg_malloc_attr(list_concat_unique_int), \
+ pg_malloc_attr(list_concat_unique_oid), \
+ pg_malloc_attr(list_free), \
+ pg_malloc_attr(list_free_deep), \
+ warn_unused_result))
+
+extern PG_LIST_ALLOC_ATTR List *list_make1_impl(NodeTag t, ListCell datum1);
+extern PG_LIST_ALLOC_ATTR List *list_make2_impl(NodeTag t, ListCell datum1, ListCell datum2);
+extern PG_LIST_ALLOC_ATTR List *list_make3_impl(NodeTag t, ListCell datum1, ListCell datum2,
+ ListCell datum3);
+extern PG_LIST_ALLOC_ATTR List *list_make4_impl(NodeTag t, ListCell datum1, ListCell datum2,
+ ListCell datum3, ListCell datum4);
+extern PG_LIST_ALLOC_ATTR List *list_make5_impl(NodeTag t, ListCell datum1, ListCell datum2,
+ ListCell datum3, ListCell datum4,
+ ListCell datum5);
+
+extern PG_LIST_ALLOC_ATTR List *lappend(List *list, void *datum);
+extern PG_LIST_ALLOC_ATTR List *lappend_int(List *list, int datum);
+extern PG_LIST_ALLOC_ATTR List *lappend_oid(List *list, Oid datum);
+extern PG_LIST_ALLOC_ATTR List *lappend_xid(List *list, TransactionId datum);
+
+extern PG_LIST_ALLOC_ATTR List *list_insert_nth(List *list, int pos, void *datum);
+extern PG_LIST_ALLOC_ATTR List *list_insert_nth_int(List *list, int pos, int datum);
+extern PG_LIST_ALLOC_ATTR List *list_insert_nth_oid(List *list, int pos, Oid datum);
+
+extern PG_LIST_ALLOC_ATTR List *lcons(void *datum, List *list);
+extern PG_LIST_ALLOC_ATTR List *lcons_int(int datum, List *list);
+extern PG_LIST_ALLOC_ATTR List *lcons_oid(Oid datum, List *list);
+
+extern PG_LIST_ALLOC_ATTR List *list_concat(List *list1, const List *list2);
+extern PG_LIST_ALLOC_ATTR List *list_concat_copy(const List *list1, const List *list2);
+
+extern PG_LIST_ALLOC_ATTR List *list_truncate(List *list, int new_size);
+
+extern PG_LIST_ALLOC_ATTR List *list_delete(List *list, void *datum);
+extern PG_LIST_ALLOC_ATTR List *list_delete_ptr(List *list, void *datum);
+extern PG_LIST_ALLOC_ATTR List *list_delete_int(List *list, int datum);
+extern PG_LIST_ALLOC_ATTR List *list_delete_oid(List *list, Oid datum);
+extern PG_LIST_ALLOC_ATTR List *list_delete_first(List *list);
+extern PG_LIST_ALLOC_ATTR List *list_delete_last(List *list);
+extern PG_LIST_ALLOC_ATTR List *list_delete_first_n(List *list, int n);
+extern PG_LIST_ALLOC_ATTR List *list_delete_nth_cell(List *list, int n);
+extern PG_LIST_ALLOC_ATTR List *list_delete_cell(List *list, ListCell *cell);
+
+extern PG_LIST_ALLOC_ATTR List *list_union(const List *list1, const List *list2);
+extern PG_LIST_ALLOC_ATTR List *list_union_ptr(const List *list1, const List *list2);
+extern PG_LIST_ALLOC_ATTR List *list_union_int(const List *list1, const List *list2);
+extern PG_LIST_ALLOC_ATTR List *list_union_oid(const List *list1, const List *list2);
+
+extern PG_LIST_ALLOC_ATTR List *list_intersection(const List *list1, const List *list2);
+extern PG_LIST_ALLOC_ATTR List *list_intersection_int(const List *list1, const List *list2);
+
+extern PG_LIST_ALLOC_ATTR List *list_append_unique(List *list, void *datum);
+extern PG_LIST_ALLOC_ATTR List *list_append_unique_ptr(List *list, void *datum);
+extern PG_LIST_ALLOC_ATTR List *list_append_unique_int(List *list, int datum);
+extern PG_LIST_ALLOC_ATTR List *list_append_unique_oid(List *list, Oid datum);
+
+extern PG_LIST_ALLOC_ATTR List *list_concat_unique(List *list1, const List *list2);
+extern PG_LIST_ALLOC_ATTR List *list_concat_unique_ptr(List *list1, const List *list2);
+extern PG_LIST_ALLOC_ATTR List *list_concat_unique_int(List *list1, const List *list2);
+extern PG_LIST_ALLOC_ATTR List *list_concat_unique_oid(List *list1, const List *list2);
+
+extern PG_LIST_ALLOC_ATTR List *list_copy(const List *oldlist);
+extern PG_LIST_ALLOC_ATTR List *list_copy_head(const List *oldlist, int len);
+extern PG_LIST_ALLOC_ATTR List *list_copy_tail(const List *oldlist, int nskip);
+extern PG_LIST_ALLOC_ATTR List *list_copy_deep(const List *oldlist);
+
#endif /* PG_LIST_H */
diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h
index d1146c12351..a9e8063023f 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -65,25 +65,40 @@ extern PGDLLIMPORT MemoryContext CurrentMemoryContext;
#define MCXT_ALLOC_NO_OOM 0x02 /* no failure if out-of-memory */
#define MCXT_ALLOC_ZERO 0x04 /* zero allocated memory */
+#define pg_alloc_attributes(size_at) \
+ __attribute__((malloc, pg_malloc_attr_i(pfree, 1), alloc_size(size_at), assume_aligned(MAXIMUM_ALIGNOF), returns_nonnull, warn_unused_result))
+#define pg_alloc_noerr_attributes(size_at) \
+ __attribute__((malloc, pg_malloc_attr_i(pfree, 1), alloc_size(size_at), assume_aligned(MAXIMUM_ALIGNOF), warn_unused_result))
+#define pg_realloc_attributes(old_at, size_at) \
+ __attribute__((alloc_size(size_at), assume_aligned(MAXIMUM_ALIGNOF), \
+ nonnull(old_at), returns_nonnull, warn_unused_result))
+#define pg_realloc_noerr_attributes(old_at, size_at) \
+ __attribute__((alloc_size(size_at), assume_aligned(MAXIMUM_ALIGNOF), \
+ nonnull(old_at), warn_unused_result))
+#define pg_dup_attributes(source_at) \
+ __attribute__((malloc, pg_malloc_attr_i(pfree, 1), assume_aligned(MAXIMUM_ALIGNOF), returns_nonnull, nonnull(source_at), warn_unused_result))
+
+extern void pfree(void *pointer);
+
/*
* Fundamental memory-allocation operations (more are in utils/memutils.h)
*/
-extern void *MemoryContextAlloc(MemoryContext context, Size size);
-extern void *MemoryContextAllocZero(MemoryContext context, Size size);
-extern void *MemoryContextAllocZeroAligned(MemoryContext context, Size size);
+extern void *MemoryContextAlloc(MemoryContext context, Size size) pg_alloc_attributes(2);
+extern void *MemoryContextAllocZero(MemoryContext context, Size size) pg_alloc_attributes(2);
+extern void *MemoryContextAllocZeroAligned(MemoryContext context, Size size) pg_alloc_attributes(2);
extern void *MemoryContextAllocExtended(MemoryContext context,
- Size size, int flags);
+ Size size, int flags) pg_alloc_noerr_attributes(2);
extern void *MemoryContextAllocAligned(MemoryContext context,
- Size size, Size alignto, int flags);
+ Size size, Size alignto, int flags) pg_alloc_noerr_attributes(2);
-extern void *palloc(Size size);
-extern void *palloc0(Size size);
-extern void *palloc_extended(Size size, int flags);
-extern void *palloc_aligned(Size size, Size alignto, int flags);
-extern pg_nodiscard void *repalloc(void *pointer, Size size);
+extern void *palloc(Size size) pg_alloc_attributes(1);
+extern void *palloc0(Size size) pg_alloc_attributes(1);
+extern void *palloc_extended(Size size, int flags) pg_alloc_noerr_attributes(1);
+extern void *palloc_aligned(Size size, Size alignto, int flags) pg_alloc_noerr_attributes(1);
+extern pg_nodiscard void *repalloc(void *pointer, Size size) pg_realloc_attributes(1, 2);
extern pg_nodiscard void *repalloc_extended(void *pointer,
- Size size, int flags);
-extern pg_nodiscard void *repalloc0(void *pointer, Size oldsize, Size size);
+ Size size, int flags) pg_realloc_noerr_attributes(1, 2);
+extern pg_nodiscard void *repalloc0(void *pointer, Size oldsize, Size size) pg_realloc_attributes(1, 2);
extern void pfree(void *pointer);
/*
@@ -123,8 +138,8 @@ extern void pfree(void *pointer);
MemoryContextAllocZero(CurrentMemoryContext, sz) )
/* Higher-limit allocators. */
-extern void *MemoryContextAllocHuge(MemoryContext context, Size size);
-extern pg_nodiscard void *repalloc_huge(void *pointer, Size size);
+extern void *MemoryContextAllocHuge(MemoryContext context, Size size) pg_alloc_attributes(2);
+extern pg_nodiscard void *repalloc_huge(void *pointer, Size size) pg_realloc_attributes(1, 2);
/*
* Although this header file is nominally backend-only, certain frontend
@@ -152,14 +167,14 @@ extern void MemoryContextRegisterResetCallback(MemoryContext context,
* These are like standard strdup() except the copied string is
* allocated in a context, not with malloc().
*/
-extern char *MemoryContextStrdup(MemoryContext context, const char *string);
-extern char *pstrdup(const char *in);
-extern char *pnstrdup(const char *in, Size len);
+extern char *MemoryContextStrdup(MemoryContext context, const char *string) pg_dup_attributes(2);
+extern char *pstrdup(const char *in) pg_dup_attributes(1);
+extern char *pnstrdup(const char *in, Size len) pg_dup_attributes(1);
-extern char *pchomp(const char *in);
+extern char *pchomp(const char *in) pg_dup_attributes(1);
/* sprintf into a palloc'd buffer --- these are in psprintf.c */
-extern char *psprintf(const char *fmt,...) pg_attribute_printf(1, 2);
-extern size_t pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) pg_attribute_printf(3, 0);
+extern char *psprintf(const char *fmt,...) pg_attribute_printf(1, 2) __attribute__((malloc, returns_nonnull, warn_unused_result));
+extern size_t pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) pg_attribute_printf(3, 0) __attribute__((warn_unused_result));
#endif /* PALLOC_H */
--
2.38.0
On 26.06.23 21:54, Andres Freund wrote:
For something like pg_list.h the malloc(free) attribute is a bit awkward to
use, because one a) needs to list ~30 functions that can free a list and b)
the referenced functions need to be declared.
Hmm. Saying list_concat() "deallocates" a list is mighty confusing
because 1) it doesn't, and 2) it might actually allocate a new list. So
while you get the useful behavior of "you probably didn't mean to use
this variable again after passing it into list_concat()", if some other
tool actually took these allocate/deallocate decorations at face value
and did a memory leak analysis with them, they'd get completely bogus
results.
I also added such attributes to bitmapset.h and palloc() et al, but that
didn't find existing bugs. It does find use-after-free instances if I add
some, similarly it does find cases of mismatching palloc with free etc.
This seems more straightforward. Even if it didn't find any bugs, I'd
imagine it would be useful during development.
Do others think this would be useful enough to be worth polishing? And do you
agree the warnings above are bugs?
I actually just played with this the other day, because I can never
remember termPQExpBuffer() vs. destroyPQExpBuffer(). I couldn't quite
make it work for that, but I found the feature overall useful, so I'd
welcome support for it.
Hi,
On 2023-06-28 10:40:22 +0200, Peter Eisentraut wrote:
On 26.06.23 21:54, Andres Freund wrote:
For something like pg_list.h the malloc(free) attribute is a bit awkward to
use, because one a) needs to list ~30 functions that can free a list and b)
the referenced functions need to be declared.Hmm. Saying list_concat() "deallocates" a list is mighty confusing because
1) it doesn't, and 2) it might actually allocate a new list.
list_concat() basically behaves like realloc(), except that the "pointer is
still valid" case is much more common. And the way that's modelled in the
annotations is to say a function frees and allocates.
Note that the free attribute references the first element for list_concat(),
not the second.
So while you get the useful behavior of "you probably didn't mean to use
this variable again after passing it into list_concat()", if some other tool
actually took these allocate/deallocate decorations at face value and did a
memory leak analysis with them, they'd get completely bogus results.
How would the annotations possibly lead to a bogus result? I see neither how
it could lead to false negatives nor false positives?
The gcc attributes are explicitly intended to track not just plain memory
allocations, the example in the docs
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
is to add them for fopen() etc. So I don't think it's likely that external
tools will interpret this is a much more stringent way.
I also added such attributes to bitmapset.h and palloc() et al, but that
didn't find existing bugs. It does find use-after-free instances if I add
some, similarly it does find cases of mismatching palloc with free etc.This seems more straightforward. Even if it didn't find any bugs, I'd
imagine it would be useful during development.
Agreed. Given our testing regimen (valgrind etc), I'd expect to find many such
bugs before long in the tree anyway. But it's much nicer to get that far. And
to find paths that aren't covered by tests.
Do others think this would be useful enough to be worth polishing? And do you
agree the warnings above are bugs?I actually just played with this the other day, because I can never remember
termPQExpBuffer() vs. destroyPQExpBuffer().
That's a pretty nasty one :(
I couldn't quite make it work for that, but I found the feature overall
useful, so I'd welcome support for it.
Yea, I don't think the attributes can comfortable handle initPQExpBuffer()
style allocation. It's somewhat posible by moving the allocation to an inline
function, and then making the thing that's allocated ->data. But it ends up
pretty messy, particularly because we need ABI stability for pqexpbuffer.h.
But createPQExpBuffer() can be dealt with reasonably.
Doing so points out:
[51/354 42 14%] Compiling C object src/bin/initdb/initdb.p/initdb.c.o
../../../../home/andres/src/postgresql/src/bin/initdb/initdb.c: In function ‘replace_guc_value’:
../../../../home/andres/src/postgresql/src/bin/initdb/initdb.c:566:9: warning: ‘free’ called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc]
566 | free(newline); /* but don't free newline->data */
| ^~~~~~~~~~~~~
../../../../home/andres/src/postgresql/src/bin/initdb/initdb.c:470:31: note: returned from ‘createPQExpBuffer’
470 | PQExpBuffer newline = createPQExpBuffer();
| ^~~~~~~~~~~~~~~~~~~
which is intentional, but ... not pretty, and could very well be a bug in
other cases. If we want to do stuff like that, we'd probably better off
having a dedicated version of destroyPQExpBuffer(). Although here it looks
like the code should just use an on-stack PQExpBuffer.
Greetings,
Andres Freund
On 28.06.23 20:15, Andres Freund wrote:
On 2023-06-28 10:40:22 +0200, Peter Eisentraut wrote:
On 26.06.23 21:54, Andres Freund wrote:
For something like pg_list.h the malloc(free) attribute is a bit awkward to
use, because one a) needs to list ~30 functions that can free a list and b)
the referenced functions need to be declared.Hmm. Saying list_concat() "deallocates" a list is mighty confusing because
1) it doesn't, and 2) it might actually allocate a new list.list_concat() basically behaves like realloc(), except that the "pointer is
still valid" case is much more common. And the way that's modelled in the
annotations is to say a function frees and allocates.Note that the free attribute references the first element for list_concat(),
not the second.
Yeah, I think that would be ok. I was worried about the cases where it
doesn't actually free the first argument, but in all those cases it
passes it as a result, so as far as a caller is concerned, it would
appear as freed and allocated, even if it's really the same.