[PATCH] Allow talloc reparenting in a destructor.

[Jeremy Allison]

Here is a proposed addition to talloc (plus tests for it).

If a destructor returns failure (-1) when freeing a child, talloc
reparents the child so the memory doesn't get lost.

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 reparents 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 then it doesn't overwrite the new parent.

Please review and push.

Cheers,

	Jeremy.
-------------- next part --------------
From 90068da67ef4a56f438e885bde81dc8831898986 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 1/2] 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>
---
 lib/talloc/talloc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index fa56ea5..9e5ba11 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -1464,6 +1464,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 186d4abe2365b4f42d0f405e1b75ea52bea39429 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 2/2] lib: talloc: Test suite for the new destructor reparent
 logic.

Signed-off-by: Jeremy Allison <jra 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..6c3cfff 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_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);
+	talloc_set_destructor(level3, NULL);
+
+	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