r13915: Fixed a very interesting class of realloc() bugs found by Coverity.

realloc can return NULL in one of two cases - (1) the realloc failed,
(2) realloc succeeded but the new size requested was zero, in which
case this is identical to a free() call.

The error paths dealing with these two cases should be different,
but mostly weren't. Secondly the standard idiom for dealing with
realloc when you know the new size is non-zero is the following :

 tmp = realloc(p, size);
 if (!tmp) {
    SAFE_FREE(p);
    return error;
 } else {
    p = tmp;
 }

However, there were *many* *many* places in Samba where we were
using the old (broken) idiom of :

 p = realloc(p, size)
 if (!p) {
    return error;
 }

which will leak the memory pointed to by p on realloc fail.

This commit (hopefully) fixes all these cases by moving to
a standard idiom of :

 p = SMB_REALLOC(p, size)
 if (!p) {
    return error;
 }

Where if the realloc returns null due to the realloc failing
or size == 0 we *guarentee* that the storage pointed to by p
has been freed. This allows me to remove a lot of code that
was dealing with the standard (more verbose) method that required
a tmp pointer. This is almost always what you want. When a
realloc fails you never usually want the old memory, you
want to free it and get into your error processing asap.

For the 11 remaining cases where we really do need to keep the
old pointer I have invented the new macro SMB_REALLOC_KEEP_OLD_ON_ERROR,
which can be used as follows :

 tmp = SMB_REALLOC_KEEP_OLD_ON_ERROR(p, size);
 if (!tmp) {
    SAFE_FREE(p);
    return error;
 } else {
    p = tmp;
 }

SMB_REALLOC_KEEP_OLD_ON_ERROR guarentees never to free the
pointer p, even on size == 0 or realloc fail. All this is
done by a hidden extra argument to Realloc(), BOOL free_old_on_error
which is set appropriately by the SMB_REALLOC and SMB_REALLOC_KEEP_OLD_ON_ERROR
macros (and their array counterparts).

It remains to be seen what this will do to our Coverity bug count :-).

Jeremy.
(This used to be commit 1d710d06a214f3f1740e80e0bffd6aab44aac2b0)

4 files changed, 24 insertions, 46 deletions

diff --git a/source3/nsswitch/wb_client.c b/source3/nsswitch/wb_client.c
index ff0f15a122..b2db25c31b 100644
--- a/source3/nsswitch/wb_client.c
+++ b/source3/nsswitch/wb_client.c
@@ -335,7 +335,7 @@ static int wb_getgroups(const char *user, gid_t **groups)
 
 int winbind_initgroups(char *user, gid_t gid)
 {
-	gid_t *tgr, *groups = NULL;
+	gid_t *groups = NULL;
 	int result;
 
 	/* Call normal initgroups if we are a local user */
@@ -364,14 +364,12 @@ int winbind_initgroups(char *user, gid_t gid)
 		/* Add group to list if necessary */
 
 		if (!is_member) {
-			tgr = SMB_REALLOC_ARRAY(groups, gid_t, ngroups + 1);
-			
-			if (!tgr) {
+			groups = SMB_REALLOC_ARRAY(groups, gid_t, ngroups + 1);
+			if (!groups) {
 				errno = ENOMEM;
 				result = -1;
 				goto done;
 			}
-			else groups = tgr;
 
 			groups[ngroups] = gid;
 			ngroups++;
diff --git a/source3/nsswitch/winbindd_cache.c b/source3/nsswitch/winbindd_cache.c
index 799818198c..7f14f359da 100644
--- a/source3/nsswitch/winbindd_cache.c
+++ b/source3/nsswitch/winbindd_cache.c
@@ -560,16 +560,14 @@ static struct cache_entry *wcache_fetch(struct winbind_cache *cache,
 */
 static void centry_expand(struct cache_entry *centry, uint32 len)
 {
-	uint8 *p;
 	if (centry->len - centry->ofs >= len)
 		return;
 	centry->len *= 2;
-	p = SMB_REALLOC(centry->data, centry->len);
-	if (!p) {
+	centry->data = SMB_REALLOC(centry->data, centry->len);
+	if (!centry->data) {
 		DEBUG(0,("out of memory: needed %d bytes in centry_expand\n", centry->len));
 		smb_panic("out of memory in centry_expand");
 	}
-	centry->data = p;
 }
 
 /*
diff --git a/source3/nsswitch/winbindd_group.c b/source3/nsswitch/winbindd_group.c
index 1ddc734703..6e125c4330 100644
--- a/source3/nsswitch/winbindd_group.c
+++ b/source3/nsswitch/winbindd_group.c
@@ -494,7 +494,7 @@ static BOOL get_sam_group_entries(struct getent_state *ent)
 {
 	NTSTATUS status;
 	uint32 num_entries;
-	struct acct_info *name_list = NULL, *tmp_name_list = NULL;
+	struct acct_info *name_list = NULL;
 	TALLOC_CTX *mem_ctx;
 	BOOL result = False;
 	struct acct_info *sam_grp_entries = NULL;
@@ -569,17 +569,14 @@ static BOOL get_sam_group_entries(struct getent_state *ent)
 		/* Copy entries into return buffer */
 
 		if ( num_entries ) {
-			if ( !(tmp_name_list = SMB_REALLOC_ARRAY( name_list, struct acct_info, ent->num_sam_entries+num_entries)) )
+			if ( !(name_list = SMB_REALLOC_ARRAY( name_list, struct acct_info, ent->num_sam_entries+num_entries)) )
 			{
 				DEBUG(0,("get_sam_group_entries: Failed to realloc more memory for %d local groups!\n", 
 					num_entries));
 				result = False;
-				SAFE_FREE( name_list );
 				goto done;
 			}
 			
-			name_list = tmp_name_list;
-				
 			memcpy( &name_list[ent->num_sam_entries], sam_grp_entries, 
 				num_entries * sizeof(struct acct_info) );
 		}
@@ -610,7 +607,7 @@ void winbindd_getgrent(struct winbindd_cli_state *state)
 	struct getent_state *ent;
 	struct winbindd_gr *group_list = NULL;
 	int num_groups, group_list_ndx = 0, i, gr_mem_list_len = 0;
-	char *new_extra_data, *gr_mem_list = NULL;
+	char *gr_mem_list = NULL;
 
 	DEBUG(3, ("[%5lu]: getgrent\n", (unsigned long)state->pid));
 
@@ -651,7 +648,7 @@ void winbindd_getgrent(struct winbindd_cli_state *state)
 		uint32 result;
 		gid_t group_gid;
 		size_t gr_mem_len;
-		char *gr_mem, *new_gr_mem_list;
+		char *gr_mem;
 		DOM_SID group_sid;
 		struct winbindd_domain *domain;
 				
@@ -766,11 +763,10 @@ void winbindd_getgrent(struct winbindd_cli_state *state)
 
 		if (result) {
 			/* Append to group membership list */
-			new_gr_mem_list = SMB_REALLOC( gr_mem_list, gr_mem_list_len + gr_mem_len);
+			gr_mem_list = SMB_REALLOC( gr_mem_list, gr_mem_list_len + gr_mem_len);
 
-			if (!new_gr_mem_list && (group_list[group_list_ndx].num_gr_mem != 0)) {
+			if (!gr_mem_list) {
 				DEBUG(0, ("out of memory\n"));
-				SAFE_FREE(gr_mem_list);
 				gr_mem_list_len = 0;
 				break;
 			}
@@ -778,8 +774,6 @@ void winbindd_getgrent(struct winbindd_cli_state *state)
 			DEBUG(10, ("list_len = %d, mem_len = %d\n",
 				   gr_mem_list_len, gr_mem_len));
 
-			gr_mem_list = new_gr_mem_list;
-
 			memcpy(&gr_mem_list[gr_mem_list_len], gr_mem,
 			       gr_mem_len);
 
@@ -817,21 +811,18 @@ void winbindd_getgrent(struct winbindd_cli_state *state)
 	if (group_list_ndx == 0)
 		goto done;
 
-	new_extra_data = SMB_REALLOC(
+	state->response.extra_data = SMB_REALLOC(
 		state->response.extra_data,
 		group_list_ndx * sizeof(struct winbindd_gr) + gr_mem_list_len);
 
-	if (!new_extra_data) {
+	if (!state->response.extra_data) {
 		DEBUG(0, ("out of memory\n"));
 		group_list_ndx = 0;
-		SAFE_FREE(state->response.extra_data);
 		SAFE_FREE(gr_mem_list);
 		request_error(state);
 		return;
 	}
 
-	state->response.extra_data = new_extra_data;
-
 	memcpy(&((char *)state->response.extra_data)
 	       [group_list_ndx * sizeof(struct winbindd_gr)], 
 	       gr_mem_list, gr_mem_list_len);
@@ -861,7 +852,6 @@ void winbindd_list_groups(struct winbindd_cli_state *state)
 	struct winbindd_domain *domain;
 	const char *which_domain;
 	char *extra_data = NULL;
-	char *ted = NULL;
 	unsigned int extra_data_len = 0, i;
 
 	DEBUG(3, ("[%5lu]: list groups\n", (unsigned long)state->pid));
@@ -901,15 +891,13 @@ void winbindd_list_groups(struct winbindd_cli_state *state)
 		
 		/* Allocate some memory for extra data.  Note that we limit
 		   account names to sizeof(fstring) = 128 characters.  */		
-                ted = SMB_REALLOC(extra_data, sizeof(fstring) * total_entries);
+		extra_data = SMB_REALLOC(extra_data, sizeof(fstring) * total_entries);
  
-		if (!ted) {
+		if (!extra_data) {
 			DEBUG(0,("failed to enlarge buffer!\n"));
-			SAFE_FREE(extra_data);
 			request_error(state);
 			return;
-		} else
-			extra_data = ted;
+		}
 
 		/* Pack group list into extra data fields */
 		for (i = 0; i < groups.num_sam_entries; i++) {
diff --git a/source3/nsswitch/winbindd_user.c b/source3/nsswitch/winbindd_user.c
index 227163b447..b48284a031 100644
--- a/source3/nsswitch/winbindd_user.c
+++ b/source3/nsswitch/winbindd_user.c
@@ -553,16 +553,12 @@ static BOOL get_sam_user_entries(struct getent_state *ent, TALLOC_CTX *mem_ctx)
 					  &info);
 		
 	if (num_entries) {
-		struct getpwent_user *tnl;
+		name_list = SMB_REALLOC_ARRAY(name_list, struct getpwent_user, ent->num_sam_entries + num_entries);
 		
-		tnl = SMB_REALLOC_ARRAY(name_list, struct getpwent_user, ent->num_sam_entries + num_entries);
-		
-		if (!tnl) {
+		if (!name_list) {
 			DEBUG(0,("get_sam_user_entries realloc failed.\n"));
-			SAFE_FREE(name_list);
 			goto done;
-		} else
-			name_list = tnl;
+		}
 	}
 
 	for (i = 0; i < num_entries; i++) {
@@ -731,7 +727,7 @@ void winbindd_list_users(struct winbindd_cli_state *state)
 	WINBIND_USERINFO *info;
 	const char *which_domain;
 	uint32 num_entries = 0, total_entries = 0;
-	char *ted, *extra_data = NULL;
+	char *extra_data = NULL;
 	int extra_data_len = 0;
 	enum winbindd_result rv = WINBINDD_ERROR;
 
@@ -767,15 +763,13 @@ void winbindd_list_users(struct winbindd_cli_state *state)
 		/* Allocate some memory for extra data */
 		total_entries += num_entries;
 			
-		ted = SMB_REALLOC(extra_data, sizeof(fstring) * total_entries);
+		extra_data = SMB_REALLOC(extra_data, sizeof(fstring) * total_entries);
 			
-		if (!ted) {
+		if (!extra_data) {
 			DEBUG(0,("failed to enlarge buffer!\n"));
-			SAFE_FREE(extra_data);
 			goto done;
-		} else 
-			extra_data = ted;
-			
+		}
+
 		/* Pack user list into extra data fields */
 			
 		for (i = 0; i < num_entries; i++) {