[PATCHES v2 BUG 13865] Properly track the size of talloc objects
Christof Schmitt
cs at samba.org
Fri Apr 5 21:21:55 UTC 2019
Ping. Does anybody want to review? The pipeline now passes.
Christof
On Mon, Apr 01, 2019 at 09:29:15PM -0700, Christof Schmitt via samba-technical wrote:
> One part was missing in the lookup with the new struct.
>
> New patches attached.
>
> New pipeline:
> https://gitlab.com/samba-team/devel/samba/pipelines/54718568
>
> Christof
> From d43fcd188b35424aa260036e93534d331cea3afd Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Mon, 1 Apr 2019 15:38:59 -0700
> Subject: [PATCH 1/3] memcache: Introduce struct for storing talloc pointer
>
> This allows extending the additional data stored for talloced objects
> later.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
> lib/util/memcache.c | 35 ++++++++++++++++++++---------------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/lib/util/memcache.c b/lib/util/memcache.c
> index 819ba44b443..d0250c6b3ee 100644
> --- a/lib/util/memcache.c
> +++ b/lib/util/memcache.c
> @@ -27,6 +27,10 @@
>
> static struct memcache *global_cache;
>
> +struct memcache_talloc_value {
> + void *ptr;
> +};
> +
> struct memcache_element {
> struct rb_node rb_node;
> struct memcache_element *prev, *next;
> @@ -180,19 +184,19 @@ void *memcache_lookup_talloc(struct memcache *cache, enum memcache_number n,
> DATA_BLOB key)
> {
> DATA_BLOB value;
> - void *result;
> + struct memcache_talloc_value mtv;
>
> if (!memcache_lookup(cache, n, key, &value)) {
> return NULL;
> }
>
> - if (value.length != sizeof(result)) {
> + if (value.length != sizeof(mtv)) {
> return NULL;
> }
>
> - memcpy(&result, value.data, sizeof(result));
> + memcpy(&mtv, value.data, sizeof(mtv));
>
> - return result;
> + return mtv.ptr;
> }
>
> static void memcache_delete_element(struct memcache *cache,
> @@ -204,12 +208,12 @@ static void memcache_delete_element(struct memcache *cache,
>
> if (memcache_is_talloc(e->n)) {
> DATA_BLOB cache_key, cache_value;
> - void *ptr;
> + struct memcache_talloc_value mtv;
>
> memcache_element_parse(e, &cache_key, &cache_value);
> - SMB_ASSERT(cache_value.length == sizeof(ptr));
> - memcpy(&ptr, cache_value.data, sizeof(ptr));
> - TALLOC_FREE(ptr);
> + SMB_ASSERT(cache_value.length == sizeof(mtv));
> + memcpy(&mtv, cache_value.data, sizeof(mtv));
> + TALLOC_FREE(mtv.ptr);
> }
>
> cache->size -= memcache_element_size(e->keylength, e->valuelength);
> @@ -275,10 +279,11 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
>
> if (value.length <= cache_value.length) {
> if (memcache_is_talloc(e->n)) {
> - void *ptr;
> - SMB_ASSERT(cache_value.length == sizeof(ptr));
> - memcpy(&ptr, cache_value.data, sizeof(ptr));
> - TALLOC_FREE(ptr);
> + struct memcache_talloc_value mtv;
> +
> + SMB_ASSERT(cache_value.length == sizeof(mtv));
> + memcpy(&mtv, cache_value.data, sizeof(mtv));
> + TALLOC_FREE(mtv.ptr);
> }
> /*
> * We can reuse the existing record
> @@ -334,8 +339,8 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
> void memcache_add_talloc(struct memcache *cache, enum memcache_number n,
> DATA_BLOB key, void *pptr)
> {
> + struct memcache_talloc_value mtv;
> void **ptr = (void **)pptr;
> - void *p;
>
> if (cache == NULL) {
> cache = global_cache;
> @@ -344,8 +349,8 @@ void memcache_add_talloc(struct memcache *cache, enum memcache_number n,
> return;
> }
>
> - p = talloc_move(cache, ptr);
> - memcache_add(cache, n, key, data_blob_const(&p, sizeof(p)));
> + mtv.ptr = talloc_move(cache, ptr);
> + memcache_add(cache, n, key, data_blob_const(&mtv, sizeof(mtv)));
> }
>
> void memcache_flush(struct memcache *cache, enum memcache_number n)
> --
> 2.17.0
>
>
> From 14a41c32e4f12012339d8b8c0c593939a23dba7c Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Mon, 1 Apr 2019 16:23:35 -0700
> Subject: [PATCH 2/3] memcache: Properly track the size of talloc objects
>
> With memcache_add_talloc, the talloc object becomes part of the pool and
> the memcache_element stores a pointer to the talloc object. The
> size of the the talloc object was not used when tracking the used space,
> allowing the cache to grow larger than defined in the memcache_init
> call.
>
> Fix this by adding the size of the talloc object to the used space.
>
> Also record the initial size of the talloc object for proper adjustment
> of the used space in the cache later. This is in case the size of the
> talloc object is modified while being owned by the cache (e.g.
> allocating talloc child objects). This should never happen, but better
> be safe than ending up with a broken cache usage counter.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
> lib/util/memcache.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/lib/util/memcache.c b/lib/util/memcache.c
> index d0250c6b3ee..1e616bd0e9a 100644
> --- a/lib/util/memcache.c
> +++ b/lib/util/memcache.c
> @@ -29,6 +29,7 @@ static struct memcache *global_cache;
>
> struct memcache_talloc_value {
> void *ptr;
> + size_t len;
> };
>
> struct memcache_element {
> @@ -213,6 +214,7 @@ static void memcache_delete_element(struct memcache *cache,
> memcache_element_parse(e, &cache_key, &cache_value);
> SMB_ASSERT(cache_value.length == sizeof(mtv));
> memcpy(&mtv, cache_value.data, sizeof(mtv));
> + cache->size -= mtv.len;
> TALLOC_FREE(mtv.ptr);
> }
>
> @@ -283,6 +285,7 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
>
> SMB_ASSERT(cache_value.length == sizeof(mtv));
> memcpy(&mtv, cache_value.data, sizeof(mtv));
> + cache->size -= mtv.len;
> TALLOC_FREE(mtv.ptr);
> }
> /*
> @@ -290,6 +293,14 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
> */
> memcpy(cache_value.data, value.data, value.length);
> e->valuelength = value.length;
> +
> + if (memcache_is_talloc(e->n)) {
> + struct memcache_talloc_value mtv;
> +
> + SMB_ASSERT(cache_value.length == sizeof(mtv));
> + memcpy(&mtv, cache_value.data, sizeof(mtv));
> + cache->size += mtv.len;
> + }
> return;
> }
>
> @@ -333,6 +344,13 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
> DLIST_ADD(cache->mru, e);
>
> cache->size += element_size;
> + if (memcache_is_talloc(e->n)) {
> + struct memcache_talloc_value mtv;
> +
> + SMB_ASSERT(cache_value.length == sizeof(mtv));
> + memcpy(&mtv, cache_value.data, sizeof(mtv));
> + cache->size += mtv.len;
> + }
> memcache_trim(cache);
> }
>
> @@ -349,6 +367,7 @@ void memcache_add_talloc(struct memcache *cache, enum memcache_number n,
> return;
> }
>
> + mtv.len = talloc_total_size(*ptr);
> mtv.ptr = talloc_move(cache, ptr);
> memcache_add(cache, n, key, data_blob_const(&mtv, sizeof(mtv)));
> }
> --
> 2.17.0
>
>
> From a15ae8a278f224e734c4e2566f3cbe4e483f25c7 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Thu, 28 Mar 2019 10:46:43 -0700
> Subject: [PATCH 3/3] torture: Add test for talloc size accounting in memcache
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
> source3/torture/torture.c | 70 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/source3/torture/torture.c b/source3/torture/torture.c
> index 219ac4a370c..782980dec3a 100644
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -11155,13 +11155,14 @@ static bool data_blob_equal(DATA_BLOB a, DATA_BLOB b)
> static bool run_local_memcache(int dummy)
> {
> struct memcache *cache;
> - DATA_BLOB k1, k2, k3;
> + DATA_BLOB k1, k2, k3, k4, k5;
> DATA_BLOB d1, d3;
> DATA_BLOB v1, v3;
>
> TALLOC_CTX *mem_ctx;
> char *ptr1 = NULL;
> char *ptr2 = NULL;
> + char *ptr3 = NULL;
>
> char *str1, *str2;
> size_t size1, size2;
> @@ -11187,6 +11188,8 @@ static bool run_local_memcache(int dummy)
> k1 = data_blob_const("d1", 2);
> k2 = data_blob_const("d2", 2);
> k3 = data_blob_const("d3", 2);
> + k4 = data_blob_const("d4", 2);
> + k5 = data_blob_const("d5", 2);
>
> memcache_add(cache, STAT_CACHE, k1, d1);
>
> @@ -11250,6 +11253,71 @@ static bool run_local_memcache(int dummy)
> return false;
> }
>
> + /*
> + * Test that talloc size also is accounted in memcache and
> + * causes purge of other object.
> + */
> +
> + str1 = talloc_zero_size(mem_ctx, 100);
> + str2 = talloc_zero_size(mem_ctx, 100);
> +
> + memcache_add_talloc(cache, GETWD_CACHE, k4, &str1);
> + memcache_add_talloc(cache, GETWD_CACHE, k5, &str1);
> +
> + ptr3 = memcache_lookup_talloc(cache, GETWD_CACHE, k4);
> + if (ptr3 != NULL) {
> + printf("Did find k4, should have been purged\n");
> + return false;
> + }
> +
> + /*
> + * Test that adding a duplicate non-talloced
> + * key/value on top of a talloced key/value takes account
> + * of the talloc_freed value size.
> + */
> + TALLOC_FREE(cache);
> + TALLOC_FREE(mem_ctx);
> +
> + mem_ctx = talloc_init("key_replace");
> + if (mem_ctx == NULL) {
> + return false;
> + }
> +
> + cache = memcache_init(NULL, sizeof(void *) == 8 ? 200 : 100);
> + if (cache == NULL) {
> + return false;
> + }
> +
> + /*
> + * Add a 100 byte talloced string. This will
> + * store a (4 or 8 byte) pointer and record the
> + * total talloced size.
> + */
> + str1 = talloc_zero_size(mem_ctx, 100);
> + memcache_add_talloc(cache, GETWD_CACHE, k4, &str1);
> + /*
> + * Now overwrite with a small talloced
> + * value. This should fit in the existing size
> + * and the total talloced size should be removed
> + * from the cache size.
> + */
> + str1 = talloc_zero_size(mem_ctx, 2);
> + memcache_add_talloc(cache, GETWD_CACHE, k4, &str1);
> + /*
> + * Now store a 20 byte string. If the
> + * total talloced size wasn't accounted for
> + * and removed in the overwrite, then this
> + * will evict k4.
> + */
> + str2 = talloc_zero_size(mem_ctx, 20);
> + memcache_add_talloc(cache, GETWD_CACHE, k5, &str2);
> +
> + ptr3 = memcache_lookup_talloc(cache, GETWD_CACHE, k4);
> + if (ptr3 == NULL) {
> + printf("Did not find k4, should not have been purged\n");
> + return false;
> + }
> +
> TALLOC_FREE(cache);
> TALLOC_FREE(mem_ctx);
>
> --
> 2.17.0
>
More information about the samba-technical
mailing list