[PATCHES v2 BUG 13865] Properly track the size of talloc objects
Christof Schmitt
cs at samba.org
Mon Apr 1 23:38:54 UTC 2019
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
-------------- next part --------------
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