[PATCH] Allow talloc reparenting in a destructor.
Jeremy Allison
jra at samba.org
Thu Mar 5 14:01:52 MST 2015
On Thu, Mar 05, 2015 at 05:41:06PM +0100, Volker Lendecke wrote:
> On Thu, Mar 05, 2015 at 08:38:08AM -0800, Jeremy Allison wrote:
> > Ah - picky compilers on our build machine :-).
> >
> > I need to explicitly ignore the return
> > from talloc_move() I think. Just added a (void).
>
> Re-pushed. Lets see how far we get this time.
New code exposed a bug inside smbd ! :-).
static int smbd_smb2_notify_smbreq_destructor(struct smb_request *smbreq)
was already trying to reparent a talloc'ed
object and return -1 to keep it around, and
we just got lucky that the thing it *actually*
got reparented to stayed around long enough.
In addition it found a bug calling talloc_set_destructor()
from inside an object destructor - any newly
set destructor was being overwritten by the old
one by talloc.
New fix for both issues, plus test modified
to test both problems attached.
Jeremy.
-------------- next part --------------
From 2a772c6148b409340a92c9ec733e222e429e0506 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 5 Mar 2015 12:48:47 -0800
Subject: [PATCH 1/3] lib: talloc: Fix bug when calling a destructor.
If the destructor itself calls talloc_set_destructor()
and returns -1, the new destructor set is overwritten
by talloc.
Dectect that and leave the new destructor in place.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
lib/talloc/talloc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index fa56ea5..1ccb039 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -991,7 +991,13 @@ static inline int _talloc_free_internal(void *ptr, const char *location)
}
tc->destructor = (talloc_destructor_t)-1;
if (d(ptr) == -1) {
- tc->destructor = d;
+ /*
+ * Only replace the destructor pointer if
+ * calling the destructor didn't modify it.
+ */
+ if (tc->destructor == (talloc_destructor_t)-1) {
+ tc->destructor = d;
+ }
return -1;
}
tc->destructor = NULL;
--
2.2.0.rc0.207.ga3a616c
From c5861c60b959a6e716a3468327143f7751d7872e Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 3 Mar 2015 17:02:47 -0800
Subject: [PATCH 2/3] lib: talloc: Allow destructors to reparent the object
they're called on.
If a destructor returns failure (-1) when freeing a child, talloc
must then reparent the child.
Firstly it tries the owner of any reference, next the parent of the
current object calling _talloc_free_children_internal(), and finally
the null context in the last resort.
If a destructor reparented its own object, which can be a very
desirable thing to do (a destructor can make a decision it isn't
time to die yet, and as the parent may be going away it might
want to move itself to longer-term storage) then this new parent
gets overwritten by the existing reparenting logic.
This patch checks when freeing a child if it already reparented
itself, and if it did doesn't then overwrite the new parent.
Makes destructors more flexible.
Signed-off-by: Jeremy Allison <jra at samba.org>
Reviewed-by: Volker Lendecke <vl at samba.org>
---
lib/talloc/talloc.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index 1ccb039..46f10f4 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -1470,6 +1470,13 @@ static inline void _talloc_free_children_internal(struct talloc_chunk *tc,
if (p) new_parent = TC_PTR_FROM_CHUNK(p);
}
if (unlikely(_talloc_free_internal(child, location) == -1)) {
+ if (talloc_parent_chunk(child) != tc) {
+ /*
+ * Destructor already reparented this child.
+ * No further reparenting needed.
+ */
+ return;
+ }
if (new_parent == null_context) {
struct talloc_chunk *p = talloc_parent_chunk(ptr);
if (p) new_parent = TC_PTR_FROM_CHUNK(p);
--
2.2.0.rc0.207.ga3a616c
From cf0945a169f36e00eb136d7d81980f56c7eaa7d0 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 3 Mar 2015 17:12:32 -0800
Subject: [PATCH 3/3] lib: talloc: Test suite for the new destructor reparent
logic.
Signed-off-by: Jeremy Allison <jra at samba.org>
Reviewed-by: Volker Lendecke <vl at samba.org>
---
lib/talloc/testsuite.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
diff --git a/lib/talloc/testsuite.c b/lib/talloc/testsuite.c
index a878278..eb3e13d 100644
--- a/lib/talloc/testsuite.c
+++ b/lib/talloc/testsuite.c
@@ -981,6 +981,84 @@ static bool test_free_parent_deny_child(void)
return true;
}
+struct new_parent {
+ void *new_parent;
+ char val[20];
+};
+
+static int reparenting_destructor(struct new_parent *np)
+{
+ talloc_set_destructor(np, NULL);
+ (void)talloc_move(np->new_parent, &np);
+ return -1;
+}
+
+static bool test_free_parent_reparent_child(void)
+{
+ void *top = talloc_new(NULL);
+ char *level1;
+ char *alternate_level1;
+ char *level2;
+ struct new_parent *level3;
+
+ printf("test: free_parent_reparent_child\n# "
+ "TALLOC FREE PARENT REPARENT CHILD\n");
+
+ level1 = talloc_strdup(top, "level1");
+ alternate_level1 = talloc_strdup(top, "alternate_level1");
+ level2 = talloc_strdup(level1, "level2");
+ level3 = talloc(level2, struct new_parent);
+ level3->new_parent = alternate_level1;
+ memset(level3->val, 'x', sizeof(level3->val));
+
+ talloc_set_destructor(level3, reparenting_destructor);
+ talloc_free(level1);
+
+ CHECK_PARENT("free_parent_reparent_child",
+ level3, alternate_level1);
+
+ talloc_free(top);
+
+ printf("success: free_parent_reparent_child\n");
+ return true;
+}
+
+static bool test_free_parent_reparent_child_in_pool(void)
+{
+ void *top = talloc_new(NULL);
+ char *level1;
+ char *alternate_level1;
+ char *level2;
+ void *pool;
+ struct new_parent *level3;
+
+ printf("test: free_parent_reparent_child_in_pool\n# "
+ "TALLOC FREE PARENT REPARENT CHILD IN POOL\n");
+
+ pool = talloc_pool(top, 1024);
+ level1 = talloc_strdup(pool, "level1");
+ alternate_level1 = talloc_strdup(top, "alternate_level1");
+ level2 = talloc_strdup(level1, "level2");
+ level3 = talloc(level2, struct new_parent);
+ level3->new_parent = alternate_level1;
+ memset(level3->val, 'x', sizeof(level3->val));
+
+ talloc_set_destructor(level3, reparenting_destructor);
+ talloc_free(level1);
+ talloc_set_destructor(level3, NULL);
+
+ CHECK_PARENT("free_parent_reparent_child_in_pool",
+ level3, alternate_level1);
+
+ /* Even freeing alternate_level1 should leave pool alone. */
+ talloc_free(alternate_level1);
+ talloc_free(top);
+
+ printf("success: free_parent_reparent_child_in_pool\n");
+ return true;
+}
+
+
static bool test_talloc_ptrtype(void)
{
void *top = talloc_new(NULL);
@@ -1674,6 +1752,10 @@ bool torture_local_talloc(struct torture_context *tctx)
test_reset();
ret &= test_free_parent_deny_child();
test_reset();
+ ret &= test_free_parent_reparent_child();
+ test_reset();
+ ret &= test_free_parent_reparent_child_in_pool();
+ test_reset();
ret &= test_talloc_ptrtype();
test_reset();
ret &= test_talloc_free_in_destructor();
--
2.2.0.rc0.207.ga3a616c
More information about the samba-technical
mailing list