[PATCHES v2 BUG 13865] Properly track the size of talloc objects
Christof Schmitt
cs at samba.org
Mon Apr 1 23:39:31 UTC 2019
Pipeline is running at
https://gitlab.com/samba-team/devel/samba/pipelines/54698070
On Mon, Apr 01, 2019 at 04:38:53PM -0700, Christof Schmitt wrote:
> Here is an updated version of the memcache patches. It fixes the problem
> Jeremy encountered. The problem was that in the codepath that replaced
> an existing talloc object, the size of the old talloc object has been
> subtracted from the cache size, but the size of the new one was not
> added. Sorry for missing this case.
>
> This patch also tracks the size of the talloc object as part of
> memcache. This is a safeguard against the case that the size of the
> talloc object is modified while being owned by the cache. The proper
> place seems to only track this for talloc objects, so a data structure
> is introduced.
>
> I did not change the size of the cache, but that can be easily added.
>
> Finally, i skipped the doc patch, as i also have another memcache
> cleanup for upstream-only. I will send those once the size patches have
> been finalized.
>
> Christof
> From a4601652419a9e97fd641207180d71b8dcef699b 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 | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/lib/util/memcache.c b/lib/util/memcache.c
> index 819ba44b443..b573ae1d2c4 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;
> @@ -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 9cc74c8f4b61baf3b8e160f5cc011ae24a412ded 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 b573ae1d2c4..9257a96e095 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 1342a1f5d9979de328f0ae90b9a8d5b3e633f89b 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