From 02a7dba9acff2bb359c322217d0c8a8bb38105ab Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 20 Oct 2020 12:18:10 -0700 Subject: [PATCH 1/5] lib: talloc: Cleanup. Use consistent preprocessor logic macros. Match other use of ALWAYS_REALLOC. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14540 Signed-off-by: Jeremy Allison --- lib/talloc/talloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index e476f3e2d05..54250c1b67d 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -1898,7 +1898,7 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons */ _talloc_chunk_set_free(tc, NULL); -#if ALWAYS_REALLOC +#if (ALWAYS_REALLOC != 0) if (pool_hdr) { new_ptr = tc_alloc_pool(tc, size + TC_HDR_SIZE, 0); pool_hdr->object_count--; -- 2.25.1 From beeacae9c1b666a1eea31fb76ca08e0ccdc7d254 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 20 Oct 2020 12:14:58 -0700 Subject: [PATCH 2/5] lib: talloc: Fix pool object accounting when doing talloc_realloc() in the ALWAYS_REALLOC compiled case. tc_alloc_pool() or the fallback malloc can return NULL. Wait until we know we are returning a valid pointer before decrementing pool_hdr->object_count due to reallocing out of the talloc_pool. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14540 Signed-off-by: Jeremy Allison --- lib/talloc/talloc.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index 54250c1b67d..885705234d4 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -1901,8 +1901,6 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons #if (ALWAYS_REALLOC != 0) if (pool_hdr) { new_ptr = tc_alloc_pool(tc, size + TC_HDR_SIZE, 0); - pool_hdr->object_count--; - if (new_ptr == NULL) { new_ptr = malloc(TC_HDR_SIZE+size); malloced = true; @@ -1912,6 +1910,11 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons if (new_ptr) { memcpy(new_ptr, tc, MIN(tc->size,size) + TC_HDR_SIZE); TC_INVALIDATE_FULL_CHUNK(tc); + /* + * Only decrement the object count in the pool once + * we know we're returning a valid new_ptr. + */ + pool_hdr->object_count--; } } else { /* We're doing malloc then free here, so record the difference. */ -- 2.25.1 From a92a9a1cd25d93237dd8617efdfe6c89e672f361 Mon Sep 17 00:00:00 2001 From: Arran Cudbard-Bell Date: Tue, 20 Oct 2020 14:10:30 -0500 Subject: [PATCH 3/5] lib: talloc: Add more debugging text for existing memlimit + pool tests BUG: https://bugzilla.samba.org/show_bug.cgi?id=14540 Signed-off-by: Arran Cudbard-Bell Reviewed-by: Jeremy Allison --- lib/talloc/testsuite.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/talloc/testsuite.c b/lib/talloc/testsuite.c index 6c9bf203b7c..571c32ca00f 100644 --- a/lib/talloc/testsuite.c +++ b/lib/talloc/testsuite.c @@ -1767,24 +1767,38 @@ static bool test_memlimit(void) talloc_free(root); /* Test memlimits with pools. */ + printf("==== talloc_pool(NULL, 10*1024)\n"); pool = talloc_pool(NULL, 10*1024); torture_assert("memlimit", pool != NULL, "failed: alloc should not fail due to memory limit\n"); + + printf("==== talloc_set_memlimit(pool, 10*1024)\n"); talloc_set_memlimit(pool, 10*1024); for (i = 0; i < 9; i++) { + printf("==== talloc_size(pool, 1024) %i/10\n", i + 1); l1 = talloc_size(pool, 1024); torture_assert("memlimit", l1 != NULL, "failed: alloc should not fail due to memory limit\n"); + talloc_report_full(pool, stdout); } /* The next alloc should fail. */ + printf("==== talloc_size(pool, 1024) 10/10\n"); l2 = talloc_size(pool, 1024); torture_assert("memlimit", l2 == NULL, "failed: alloc should fail due to memory limit\n"); + talloc_report_full(pool, stdout); + /* Moving one of the children shouldn't change the limit, as it's still inside the pool. */ + + printf("==== talloc_new(NULL)\n"); root = talloc_new(NULL); + + printf("==== talloc_steal(root, l1)\n"); talloc_steal(root, l1); + + printf("==== talloc_size(pool, 1024)\n"); l2 = talloc_size(pool, 1024); torture_assert("memlimit", l2 == NULL, "failed: alloc should fail due to memory limit\n"); -- 2.25.1 From 8f5809784662207d4fc8c8c4b6e9690b13e85171 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 20 Oct 2020 10:52:55 -0700 Subject: [PATCH 4/5] lib: talloc: Fix memlimit on pool realloc. We only have to do the memlimit check before any real malloc or realloc. Allocations out of a memory pool have already been counted in the memory limit, so don't check in those cases. This is an application-visible change (although fixing a bug) so bump the ABI to 2.3.1 -> 2.3.2. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14540 Signed-off-by: Jeremy Allison Signed-off-by: Arran Cudbard-Bell --- lib/talloc/ABI/pytalloc-util-2.3.2.sigs | 16 ++++++ lib/talloc/ABI/talloc-2.3.2.sigs | 65 +++++++++++++++++++++++ lib/talloc/talloc.c | 69 ++++++++++++++++++++++--- lib/talloc/wscript | 2 +- 4 files changed, 144 insertions(+), 8 deletions(-) create mode 100644 lib/talloc/ABI/pytalloc-util-2.3.2.sigs create mode 100644 lib/talloc/ABI/talloc-2.3.2.sigs diff --git a/lib/talloc/ABI/pytalloc-util-2.3.2.sigs b/lib/talloc/ABI/pytalloc-util-2.3.2.sigs new file mode 100644 index 00000000000..6056577960d --- /dev/null +++ b/lib/talloc/ABI/pytalloc-util-2.3.2.sigs @@ -0,0 +1,16 @@ +_pytalloc_check_type: int (PyObject *, const char *) +_pytalloc_get_mem_ctx: TALLOC_CTX *(PyObject *) +_pytalloc_get_name: const char *(PyObject *) +_pytalloc_get_ptr: void *(PyObject *) +_pytalloc_get_type: void *(PyObject *, const char *) +pytalloc_BaseObject_PyType_Ready: int (PyTypeObject *) +pytalloc_BaseObject_check: int (PyObject *) +pytalloc_BaseObject_size: size_t (void) +pytalloc_Check: int (PyObject *) +pytalloc_GenericObject_reference_ex: PyObject *(TALLOC_CTX *, void *) +pytalloc_GenericObject_steal_ex: PyObject *(TALLOC_CTX *, void *) +pytalloc_GetBaseObjectType: PyTypeObject *(void) +pytalloc_GetObjectType: PyTypeObject *(void) +pytalloc_reference_ex: PyObject *(PyTypeObject *, TALLOC_CTX *, void *) +pytalloc_steal: PyObject *(PyTypeObject *, void *) +pytalloc_steal_ex: PyObject *(PyTypeObject *, TALLOC_CTX *, void *) diff --git a/lib/talloc/ABI/talloc-2.3.2.sigs b/lib/talloc/ABI/talloc-2.3.2.sigs new file mode 100644 index 00000000000..9969ce33389 --- /dev/null +++ b/lib/talloc/ABI/talloc-2.3.2.sigs @@ -0,0 +1,65 @@ +_talloc: void *(const void *, size_t) +_talloc_array: void *(const void *, size_t, unsigned int, const char *) +_talloc_free: int (void *, const char *) +_talloc_get_type_abort: void *(const void *, const char *, const char *) +_talloc_memdup: void *(const void *, const void *, size_t, const char *) +_talloc_move: void *(const void *, const void *) +_talloc_pooled_object: void *(const void *, size_t, const char *, unsigned int, size_t) +_talloc_realloc: void *(const void *, void *, size_t, const char *) +_talloc_realloc_array: void *(const void *, void *, size_t, unsigned int, const char *) +_talloc_reference_loc: void *(const void *, const void *, const char *) +_talloc_set_destructor: void (const void *, int (*)(void *)) +_talloc_steal_loc: void *(const void *, const void *, const char *) +_talloc_zero: void *(const void *, size_t, const char *) +_talloc_zero_array: void *(const void *, size_t, unsigned int, const char *) +talloc_asprintf: char *(const void *, const char *, ...) +talloc_asprintf_append: char *(char *, const char *, ...) +talloc_asprintf_append_buffer: char *(char *, const char *, ...) +talloc_autofree_context: void *(void) +talloc_check_name: void *(const void *, const char *) +talloc_disable_null_tracking: void (void) +talloc_enable_leak_report: void (void) +talloc_enable_leak_report_full: void (void) +talloc_enable_null_tracking: void (void) +talloc_enable_null_tracking_no_autofree: void (void) +talloc_find_parent_byname: void *(const void *, const char *) +talloc_free_children: void (void *) +talloc_get_name: const char *(const void *) +talloc_get_size: size_t (const void *) +talloc_increase_ref_count: int (const void *) +talloc_init: void *(const char *, ...) +talloc_is_parent: int (const void *, const void *) +talloc_named: void *(const void *, size_t, const char *, ...) +talloc_named_const: void *(const void *, size_t, const char *) +talloc_parent: void *(const void *) +talloc_parent_name: const char *(const void *) +talloc_pool: void *(const void *, size_t) +talloc_realloc_fn: void *(const void *, void *, size_t) +talloc_reference_count: size_t (const void *) +talloc_reparent: void *(const void *, const void *, const void *) +talloc_report: void (const void *, FILE *) +talloc_report_depth_cb: void (const void *, int, int, void (*)(const void *, int, int, int, void *), void *) +talloc_report_depth_file: void (const void *, int, int, FILE *) +talloc_report_full: void (const void *, FILE *) +talloc_set_abort_fn: void (void (*)(const char *)) +talloc_set_log_fn: void (void (*)(const char *)) +talloc_set_log_stderr: void (void) +talloc_set_memlimit: int (const void *, size_t) +talloc_set_name: const char *(const void *, const char *, ...) +talloc_set_name_const: void (const void *, const char *) +talloc_show_parents: void (const void *, FILE *) +talloc_strdup: char *(const void *, const char *) +talloc_strdup_append: char *(char *, const char *) +talloc_strdup_append_buffer: char *(char *, const char *) +talloc_strndup: char *(const void *, const char *, size_t) +talloc_strndup_append: char *(char *, const char *, size_t) +talloc_strndup_append_buffer: char *(char *, const char *, size_t) +talloc_test_get_magic: int (void) +talloc_total_blocks: size_t (const void *) +talloc_total_size: size_t (const void *) +talloc_unlink: int (const void *, void *) +talloc_vasprintf: char *(const void *, const char *, va_list) +talloc_vasprintf_append: char *(char *, const char *, va_list) +talloc_vasprintf_append_buffer: char *(char *, const char *, va_list) +talloc_version_major: int (void) +talloc_version_minor: int (void) diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index 885705234d4..078e58ca352 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -1833,13 +1833,6 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons return NULL; } - if (tc->limit && (size > tc->size)) { - if (!talloc_memlimit_check(tc->limit, (size - tc->size))) { - errno = ENOMEM; - return NULL; - } - } - /* handle realloc inside a talloc_pool */ if (unlikely(tc->flags & TALLOC_FLAG_POOLMEM)) { pool_hdr = tc->pool; @@ -1902,6 +1895,25 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons if (pool_hdr) { new_ptr = tc_alloc_pool(tc, size + TC_HDR_SIZE, 0); if (new_ptr == NULL) { + /* + * Couldn't allocate from pool (pool size + * counts as already allocated for memlimit + * purposes). We must check memory limit + * before any real malloc. + */ + if (tc->limit) { + /* + * Note we're doing an extra malloc, + * on top of the pool size, so account + * for size only, not the difference + * between old and new size. + */ + if (!talloc_memlimit_check(tc->limit, size)) { + _talloc_chunk_set_not_free(tc); + errno = ENOMEM; + return NULL; + } + } new_ptr = malloc(TC_HDR_SIZE+size); malloced = true; new_size = size; @@ -1920,6 +1932,18 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons /* We're doing malloc then free here, so record the difference. */ old_size = tc->size; new_size = size; + /* + * We must check memory limit + * before any real malloc. + */ + if (tc->limit && (size > old_size)) { + if (!talloc_memlimit_check(tc->limit, + (size - old_size))) { + _talloc_chunk_set_not_free(tc); + errno = ENOMEM; + return NULL; + } + } new_ptr = malloc(size + TC_HDR_SIZE); if (new_ptr) { memcpy(new_ptr, tc, MIN(tc->size, size) + TC_HDR_SIZE); @@ -2023,6 +2047,25 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons new_ptr = tc_alloc_pool(tc, size + TC_HDR_SIZE, 0); if (new_ptr == NULL) { + /* + * Couldn't allocate from pool (pool size + * counts as already allocated for memlimit + * purposes). We must check memory limit + * before any real malloc. + */ + if (tc->limit) { + /* + * Note we're doing an extra malloc, + * on top of the pool size, so account + * for size only, not the difference + * between old and new size. + */ + if (!talloc_memlimit_check(tc->limit, size)) { + _talloc_chunk_set_not_free(tc); + errno = ENOMEM; + return NULL; + } + } new_ptr = malloc(TC_HDR_SIZE+size); malloced = true; new_size = size; @@ -2038,6 +2081,18 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons /* We're doing realloc here, so record the difference. */ old_size = tc->size; new_size = size; + /* + * We must check memory limit + * before any real realloc. + */ + if (tc->limit && (size > old_size)) { + if (!talloc_memlimit_check(tc->limit, + (size - old_size))) { + _talloc_chunk_set_not_free(tc); + errno = ENOMEM; + return NULL; + } + } new_ptr = realloc(tc, size + TC_HDR_SIZE); } got_new_ptr: diff --git a/lib/talloc/wscript b/lib/talloc/wscript index b955d215b80..a767477357f 100644 --- a/lib/talloc/wscript +++ b/lib/talloc/wscript @@ -1,7 +1,7 @@ #!/usr/bin/env python APPNAME = 'talloc' -VERSION = '2.3.1' +VERSION = '2.3.2' import os import sys -- 2.25.1 From 6df1925218664cf9edac2335f8ef124a9af5f680 Mon Sep 17 00:00:00 2001 From: Arran Cudbard-Bell Date: Tue, 20 Oct 2020 14:12:17 -0500 Subject: [PATCH 5/5] lib: talloc: More tests for realloc when used with memlimited pools This requires the previous patch. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14540 Signed-off-by: Arran Cudbard-Bell Reviewed-by: Jeremy Allison --- lib/talloc/testsuite.c | 103 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/lib/talloc/testsuite.c b/lib/talloc/testsuite.c index 571c32ca00f..6f23ad4e18a 100644 --- a/lib/talloc/testsuite.c +++ b/lib/talloc/testsuite.c @@ -1803,6 +1803,109 @@ static bool test_memlimit(void) torture_assert("memlimit", l2 == NULL, "failed: alloc should fail due to memory limit\n"); + printf("==== talloc_free_children(pool)\n"); + talloc_free(l1); + talloc_free_children(pool); + + printf("==== talloc_size(pool, 1024)\n"); + l1 = talloc_size(pool, 1024); + + /* try reallocs of increasing size */ + for (i = 1; i < 9; i++) { + printf("==== talloc_realloc_size(NULL, l1, %i*1024) %i/10\n", i, i + 1); + l1 = talloc_realloc_size(NULL, l1, i*1024); + torture_assert("memlimit", l1 != NULL, + "failed: realloc should not fail due to memory limit\n"); + talloc_report_full(pool, stdout); + } + /* The next alloc should fail. */ + printf("==== talloc_realloc_size(NULL, l1, 10*1024) 10/10\n"); + l2 = talloc_realloc_size(NULL, l1, 10*1024); + torture_assert("memlimit", l2 == NULL, + "failed: realloc should fail due to memory limit\n"); + + /* Increase the memlimit */ + printf("==== talloc_set_memlimit(pool, 11*1024)\n"); + talloc_set_memlimit(pool, 11*1024); + + /* The final realloc should still fail + as the entire realloced chunk needs to be moved out of the pool */ + printf("==== talloc_realloc_size(NULL, l1, 10*1024) 10/10\n"); + l2 = talloc_realloc_size(NULL, l1, 10*1024); + torture_assert("memlimit", l2 == NULL, + "failed: realloc should fail due to memory limit\n"); + + talloc_report_full(pool, stdout); + + printf("==== talloc_set_memlimit(pool, 21*1024)\n"); + talloc_set_memlimit(pool, 21*1024); + + /* There's now sufficient space to move the chunk out of the pool */ + printf("==== talloc_realloc_size(NULL, l1, 10*1024) 10/10\n"); + l2 = talloc_realloc_size(NULL, l1, 10*1024); + torture_assert("memlimit", l2 != NULL, + "failed: realloc should not fail due to memory limit\n"); + + talloc_report_full(pool, stdout); + + /* ...which should mean smaller allocations can now occur within the pool */ + printf("==== talloc_size(pool, 9*1024)\n"); + l1 = talloc_size(pool, 9*1024); + torture_assert("memlimit", l1 != NULL, + "failed: new allocations should be allowed in the pool\n"); + + talloc_report_full(pool, stdout); + + /* But reallocs bigger than the pool will still fail */ + printf("==== talloc_realloc_size(NULL, l1, 10*1024)\n"); + l2 = talloc_realloc_size(NULL, l1, 10*1024); + torture_assert("memlimit", l2 == NULL, + "failed: realloc should fail due to memory limit\n"); + + talloc_report_full(pool, stdout); + + /* ..as well as allocs */ + printf("==== talloc_size(pool, 1024)\n"); + l1 = talloc_size(pool, 1024); + torture_assert("memlimit", l1 == NULL, + "failed: alloc should fail due to memory limit\n"); + + talloc_report_full(pool, stdout); + + printf("==== talloc_free_children(pool)\n"); + talloc_free_children(pool); + + printf("==== talloc_set_memlimit(pool, 1024)\n"); + talloc_set_memlimit(pool, 1024); + + /* We should still be able to allocate up to the pool limit + because the memlimit only applies to new heap allocations */ + printf("==== talloc_size(pool, 9*1024)\n"); + l1 = talloc_size(pool, 9*1024); + torture_assert("memlimit", l1 != NULL, + "failed: alloc should not fail due to memory limit\n"); + + talloc_report_full(pool, stdout); + + l1 = talloc_size(pool, 1024); + torture_assert("memlimit", l1 == NULL, + "failed: alloc should fail due to memory limit\n"); + + talloc_report_full(pool, stdout); + + printf("==== talloc_free_children(pool)\n"); + talloc_free_children(pool); + + printf("==== talloc_set_memlimit(pool, 10*1024)\n"); + talloc_set_memlimit(pool, 10*1024); + + printf("==== talloc_size(pool, 1024)\n"); + l1 = talloc_size(pool, 1024); + torture_assert("memlimit", l1 != NULL, + "failed: alloc should not fail due to memory limit\n"); + + talloc_report_full(pool, stdout); + talloc_free(pool); talloc_free(root); printf("success: memlimit\n"); -- 2.25.1