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
>From e05c1260ee70457e9a899d5c32e5b85702193739 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@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
