[PATCHES v2 BUG 13865] Properly track the size of talloc objects
Christof Schmitt
cs at samba.org
Tue Apr 2 04:29:15 UTC 2019
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
-------------- next part --------------
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