r2709: finally solved the talloc reference problem.

The problem was that the simple "uint_t ref_count;" in a talloc chunk
did not give enough information. It told us that a pointer was
referenced more than once, but it didn't say who it was referenced
by. This means that when the pointer was freed we had no sane way to
clean up the reference.

I have now replaced ref_count with a "refs" list, which means that
references point to the pointer, and the pointer has a linked list of
references. So now we can cleanup from either direction without losing track of anything.

I've also added a LOCAL-TALLOC smbtorture test that tests talloc
behaviour for some common uses.
(This used to be commit 911a8d590cb184bcb892810729955c2c4cf02550)

4 files changed, 220 insertions, 48 deletions

diff --git a/source4/lib/talloc.c b/source4/lib/talloc.c
index 5d22754f5b..d0056d7b2a 100644
--- a/source4/lib/talloc.c
+++ b/source4/lib/talloc.c
@@ -32,13 +32,20 @@
 
 static const void *null_context;
 
+struct talloc_reference_handle {
+	struct talloc_reference_handle *next, *prev;
+	void *ptr;
+};
+
+typedef int (*talloc_destructor_t)(void *);
+
 struct talloc_chunk {
 	struct talloc_chunk *next, *prev;
 	struct talloc_chunk *parent, *child;
+	struct talloc_reference_handle *refs;
 	size_t size;
 	uint_t magic;
-	uint_t ref_count;
-	int (*destructor)(void *);
+	talloc_destructor_t destructor;
 	const char *name;
 };
 
@@ -78,10 +85,10 @@ void *_talloc(const void *context, size_t size)
 
 	tc->size = size;
 	tc->magic = TALLOC_MAGIC;
-	tc->ref_count = 1;
 	tc->destructor = NULL;
 	tc->child = NULL;
 	tc->name = NULL;
+	tc->refs = NULL;
 
 	if (context) {
 		struct talloc_chunk *parent = talloc_chunk_from_ptr(context);
@@ -114,14 +121,11 @@ void talloc_set_destructor(const void *ptr, int (*destructor)(void *))
 }
 
 /*
-  increase the reference count on a piece of memory. To decrease the 
-  reference count call talloc_free(), which will free the memory if 
-  the reference count reaches zero
+  increase the reference count on a piece of memory. 
 */
 void talloc_increase_ref_count(const void *ptr)
 {
-	struct talloc_chunk *tc = talloc_chunk_from_ptr(ptr);
-	tc->ref_count++;
+	talloc_reference(null_context, ptr);
 }
 
 /*
@@ -129,8 +133,14 @@ void talloc_increase_ref_count(const void *ptr)
 */
 static int talloc_reference_destructor(void *ptr)
 {
-	void **handle = ptr;
-	talloc_free(*handle);
+	struct talloc_reference_handle *handle = ptr;
+	struct talloc_chunk *tc1 = talloc_chunk_from_ptr(ptr);
+	struct talloc_chunk *tc2 = talloc_chunk_from_ptr(handle->ptr);
+	if (tc1->destructor != (talloc_destructor_t)-1) {
+		tc1->destructor = NULL;
+	}
+	DLIST_REMOVE(tc2->refs, handle);
+	talloc_free(handle);
 	return 0;
 }
 
@@ -145,8 +155,9 @@ static int talloc_reference_destructor(void *ptr)
 */
 void *talloc_reference(const void *context, const void *ptr)
 {
-	void **handle;
-	handle = talloc_named_const(context, sizeof(void *), ".reference");
+	struct talloc_chunk *tc = talloc_chunk_from_ptr(ptr);
+	struct talloc_reference_handle *handle;
+	handle = talloc_named_const(context, sizeof(*handle), ".reference");
 	if (handle == NULL) {
 		return NULL;
 	}
@@ -154,9 +165,9 @@ void *talloc_reference(const void *context, const void *ptr)
 	   main context as that allows the caller to still setup their
 	   own destructor on the context if they want to */
 	talloc_set_destructor(handle, talloc_reference_destructor);
-	talloc_increase_ref_count(ptr);
-	*handle = discard_const(ptr);
-	return *handle;
+	handle->ptr = discard_const(ptr);
+	DLIST_ADD(tc->refs, handle);
+	return handle->ptr;
 }
 
 
@@ -280,7 +291,7 @@ void *talloc_init(const char *fmt, ...) _PRINTF_ATTRIBUTE(1,2)
 */
 int talloc_free(void *ptr)
 {
-	struct talloc_chunk *tc, *tc2, *next;
+	struct talloc_chunk *tc;
 
 	if (ptr == NULL) {
 		return -1;
@@ -288,26 +299,29 @@ int talloc_free(void *ptr)
 
 	tc = talloc_chunk_from_ptr(ptr);
 
-	if (tc->ref_count > 1) {
-		tc->ref_count--;
-		return -1;
+	if (tc->refs) {
+		talloc_reference_destructor(tc->refs);
+		return 0;
 	}
 
-	/* while processing the free, increase the reference count
-	   so we don't recurse into this function */
-	tc->ref_count++;
 	if (tc->destructor) {
-		if (tc->destructor(ptr) == -1) {
-			tc->ref_count--;
+		talloc_destructor_t d = tc->destructor;
+		if (d == (talloc_destructor_t)-1) {
+			return -1;
+		}
+		tc->destructor = (talloc_destructor_t)-1;
+		if (d(ptr) == -1) {
+			tc->destructor = d;
 			return -1;
 		}
+		tc->destructor = NULL;
 	}
 
-	for (tc2=tc->child;tc2;tc2=next) {
-		next = tc2->next;
-		talloc_free(tc2 + 1);
+	while (tc->child) {
+		talloc_free(talloc_steal(tc->parent?tc->parent+1:null_context, 
+					 tc->child+1));
 	}
-	
+
 	if (tc->parent) {
 		DLIST_REMOVE(tc->parent->child, tc);
 		if (tc->parent->child) {
@@ -318,10 +332,6 @@ int talloc_free(void *ptr)
 		if (tc->next) tc->next->prev = tc->prev;
 	}
 
-	if (tc->child) {
-		tc->child->parent = tc->parent;
-	}
-
 	tc->magic = TALLOC_MAGIC_FREE;
 
 	free(tc);
@@ -352,6 +362,11 @@ void *_talloc_realloc(const void *context, void *ptr, size_t size, const char *n
 
 	tc = talloc_chunk_from_ptr(ptr);
 
+	/* don't allow realloc on referenced pointers */
+	if (tc->refs) {
+		return NULL;
+	}
+
 	/* by resetting magic we catch users of the old memory */
 	tc->magic = TALLOC_MAGIC_FREE;
 
@@ -382,7 +397,7 @@ void *_talloc_realloc(const void *context, void *ptr, size_t size, const char *n
 
 /* 
    move a lump of memory from one talloc context to another return the
-   ptr on success, or NUL if it could not be transferred
+   ptr on success, or NULL if it could not be transferred
 */
 void *talloc_steal(const void *new_ctx, const void *ptr)
 {
@@ -393,10 +408,26 @@ void *talloc_steal(const void *new_ctx, const void *ptr)
 	}
 
 	tc = talloc_chunk_from_ptr(ptr);
+
+	if (new_ctx == NULL) {
+		if (tc->parent) {
+			DLIST_REMOVE(tc->parent->child, tc);
+			if (tc->parent->child) {
+				tc->parent->child->parent = tc->parent;
+			}
+		} else {
+			if (tc->prev) tc->prev->next = tc->next;
+			if (tc->next) tc->next->prev = tc->prev;
+		}
+		
+		tc->parent = tc->next = tc->prev = NULL;
+		return discard_const(ptr);
+	}
+
 	new_tc = talloc_chunk_from_ptr(new_ctx);
 
 	if (tc == new_tc) {
-		discard_const(ptr);
+		return discard_const(ptr);
 	}
 
 	if (tc->parent) {
@@ -419,10 +450,19 @@ void *talloc_steal(const void *new_ctx, const void *ptr)
 /*
   return the total size of a talloc pool (subtree)
 */
-static off_t talloc_total_size(const void *ptr)
+off_t talloc_total_size(const void *ptr)
 {
 	off_t total = 0;
-	struct talloc_chunk *c, *tc = talloc_chunk_from_ptr(ptr);
+	struct talloc_chunk *c, *tc;
+	
+	if (ptr == NULL) {
+		ptr = null_context;
+	}
+	if (ptr == NULL) {
+		return 0;
+	}
+
+	tc = talloc_chunk_from_ptr(ptr);
 
 	total = tc->size;
 	for (c=tc->child;c;c=c->next) {
@@ -447,6 +487,21 @@ static off_t talloc_total_blocks(const void *ptr)
 }
 
 /*
+  return the number of external references to a pointer
+*/
+static int talloc_reference_count(const void *ptr)
+{
+	struct talloc_chunk *tc = talloc_chunk_from_ptr(ptr);
+	struct talloc_reference_handle *h;
+	int ret = 0;
+
+	for (h=tc->refs;h;h=h->next) {
+		ret++;
+	}
+	return ret;
+}
+
+/*
   report on memory usage by all children of a pointer, giving a full tree view
 */
 static void talloc_report_depth(const void *ptr, FILE *f, int depth)
@@ -456,8 +511,8 @@ static void talloc_report_depth(const void *ptr, FILE *f, int depth)
 	for (c=tc->child;c;c=c->next) {
 		const char *name = talloc_get_name(c+1);
 		if (strcmp(name, ".reference") == 0) {
-			void **handle = (void *)(c+1);
-			const char *name2 = talloc_get_name(*handle);
+			struct talloc_reference_handle *handle = (void *)(c+1);
+			const char *name2 = talloc_get_name(handle->ptr);
 			fprintf(f, "%*sreference to: %s\n", depth*4, "", name2);
 		} else {
 			fprintf(f, "%*s%-30s contains %6lu bytes in %3lu blocks (ref %d)\n", 
@@ -465,7 +520,7 @@ static void talloc_report_depth(const void *ptr, FILE *f, int depth)
 				name,
 				(unsigned long)talloc_total_size(c+1),
 				(unsigned long)talloc_total_blocks(c+1),
-				c->ref_count);
+				talloc_reference_count(c+1));
 			talloc_report_depth(c+1, f, depth+1);
 		}
 	}
diff --git a/source4/torture/config.mk b/source4/torture/config.mk
index 9c7395e572..6403d0c3a7 100644
--- a/source4/torture/config.mk
+++ b/source4/torture/config.mk
@@ -98,7 +98,8 @@ REQUIRED_SUBSYSTEMS = \
 # Start SUBSYSTEM TORTURE_LOCAL
 [SUBSYSTEM::TORTURE_LOCAL]
 ADD_OBJ_FILES = \
-		torture/local/iconv.o
+		torture/local/iconv.o \
+		torture/local/talloc.o
 REQUIRED_SUBSYSTEMS = \
 		LIBSMB
 # End SUBSYSTEM TORTURE_LOCAL
diff --git a/source4/torture/local/talloc.c b/source4/torture/local/talloc.c
new file mode 100644
index 0000000000..49cdc958b0
--- /dev/null
+++ b/source4/torture/local/talloc.c
@@ -0,0 +1,115 @@
+/* 
+   Unix SMB/CIFS implementation.
+
+   local testing of talloc routines.
+
+   Copyright (C) Andrew Tridgell 2004
+   
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+   
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+   
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+#include "includes.h"
+
+
+/*
+  test references 
+*/
+static BOOL test_ref1(void)
+{
+	void *p1, *p2, *ref, *r1;
+
+	printf("TESTING SINGLE REFERENCE FREE\n");
+
+	p1 = talloc_named_const(NULL, 1, "p1");
+	p2 = talloc_named_const(p1, 1, "p2");
+	talloc_named_const(p1, 1, "x1");
+	talloc_named_const(p1, 1, "x2");
+	talloc_named_const(p1, 1, "x3");
+
+	r1 = talloc_named_const(NULL, 1, "r1");	
+	ref = talloc_reference(r1, p2);
+	talloc_report_full(NULL, stdout);
+
+	printf("Freeing p2\n");
+	talloc_free(p2);
+	talloc_report_full(NULL, stdout);
+
+	printf("Freeing p1\n");
+	talloc_free(p1);
+	talloc_report_full(NULL, stdout);
+
+	printf("Freeing r1\n");
+	talloc_free(r1);
+	talloc_report_full(NULL, stdout);
+
+	if (talloc_total_size(NULL) != 0) {
+		printf("non-zero total size\n");
+		return False;
+	}
+
+	return True;
+}
+
+/*
+  test references 
+*/
+static BOOL test_ref2(void)
+{
+	void *p1, *p2, *ref, *r1;
+
+	printf("TESTING DOUBLE REFERENCE FREE\n");
+
+	p1 = talloc_named_const(NULL, 1, "p1");
+	talloc_named_const(p1, 1, "x1");
+	talloc_named_const(p1, 1, "x2");
+	talloc_named_const(p1, 1, "x3");
+	p2 = talloc_named_const(p1, 1, "p2");
+
+	r1 = talloc_named_const(NULL, 1, "r1");	
+	ref = talloc_reference(r1, p2);
+	talloc_report_full(NULL, stdout);
+	printf("Freeing ref\n");
+	talloc_free(ref);
+	talloc_report_full(NULL, stdout);
+	printf("Freeing p2\n");
+	talloc_free(p2);
+	talloc_report_full(NULL, stdout);
+	printf("Freeing p1\n");
+	talloc_free(p1);
+	talloc_report_full(NULL, stdout);
+	printf("Freeing r1\n");
+	talloc_free(r1);
+	talloc_report_full(NULL, stdout);
+
+	if (talloc_total_size(NULL) != 0) {
+		printf("non-zero total size\n");
+		return False;
+	}
+
+	return True;
+}
+
+
+BOOL torture_local_talloc(int dummy) 
+{
+	BOOL ret = True;
+
+	init_iconv();
+
+	ret &= test_ref1();
+	ret &= test_ref2();
+
+	return True;
+}
diff --git a/source4/torture/torture.c b/source4/torture/torture.c
index 5db861124b..c6af807357 100644
--- a/source4/torture/torture.c
+++ b/source4/torture/torture.c
@@ -94,10 +94,10 @@ BOOL torture_open_connection_share(struct smbcli_state **c,
 		flags |= SMBCLI_FULL_CONNECTION_USE_KERBEROS;
 
 	status = smbcli_full_connection(c, lp_netbios_name(),
-				     hostname, NULL, 
-				     sharename, "?????", 
-				     username, username[0]?userdomain:"",
-				     password, flags, &retry);
+					hostname, NULL, 
+					sharename, "?????", 
+					username, username[0]?userdomain:"",
+					password, flags, &retry);
 	if (!NT_STATUS_IS_OK(status)) {
 		printf("Failed to open connection - %s\n", nt_errstr(status));
 		return False;
@@ -858,10 +858,10 @@ static BOOL run_tcon_devtype_test(int dummy)
 	const char *password = lp_parm_string(-1, "torture", "password");
 	
 	status = smbcli_full_connection(&cli1, lp_netbios_name(),
-				     host, NULL, 
-				     share, "?????",
-				     username, userdomain,
-				     password, flags, &retry);
+					host, NULL, 
+					share, "?????",
+					username, userdomain,
+					password, flags, &retry);
 
 	if (!NT_STATUS_IS_OK(status)) {
 		printf("could not open connection\n");
@@ -4247,6 +4247,7 @@ static struct {
 	/* local (no server) testers */
 	{"LOCAL-NTLMSSP", torture_ntlmssp_self_check, 0},
 	{"LOCAL-ICONV", torture_local_iconv, 0},
+	{"LOCAL-TALLOC", torture_local_talloc, 0},
 
 	/* ldap testers */
 	{"LDAP-BASIC", torture_ldap_basic, 0},