Convert the prs_XXX struct and functions to use talloc instead of malloc. Passes valgrind and make tests for client and server. Second version of this patch after splitting up at Simo's request. Patch to follow will delete extraneous prs_mem_free() calls.

Jeremy.

Signed-off-by: Simo Sorce <idra@samba.org>

5 files changed, 78 insertions, 61 deletions

diff --git a/source3/include/ntdomain.h b/source3/include/ntdomain.h
index 213cb9ff75..25bef47493 100644
--- a/source3/include/ntdomain.h
+++ b/source3/include/ntdomain.h
@@ -42,7 +42,9 @@ typedef struct _prs_struct {
 	uint32 data_offset; /* Current working offset into data. */
 	uint32 buffer_size; /* Current allocated size of the buffer. */
 	uint32 grow_size; /* size requested via prs_grow() calls */
-	char *data_p; /* The buffer itself. */
+	/* The buffer itself. If "is_dynamic" is true this
+	 * MUST BE TALLOC'ed off mem_ctx. */
+	char *data_p;
 	TALLOC_CTX *mem_ctx; /* When unmarshalling, use this.... */
 } prs_struct;
 
diff --git a/source3/rpc_client/cli_pipe.c b/source3/rpc_client/cli_pipe.c
index e248133de3..d420cbce3c 100644
--- a/source3/rpc_client/cli_pipe.c
+++ b/source3/rpc_client/cli_pipe.c
@@ -1453,7 +1453,6 @@ static void rpc_api_pipe_trans_done(struct tevent_req *subreq)
 	NTSTATUS status;
 	uint8_t *rdata = NULL;
 	uint32_t rdata_len = 0;
-	char *rdata_copy;
 
 	status = cli_api_pipe_recv(subreq, state, &rdata, &rdata_len);
 	TALLOC_FREE(subreq);
@@ -1471,16 +1470,11 @@ static void rpc_api_pipe_trans_done(struct tevent_req *subreq)
 	}
 
 	/*
-	 * Give the memory received from cli_trans as dynamic to the current
-	 * pdu. Duplicating it sucks, but prs_struct doesn't know about talloc
-	 * :-(
+	 * This is equivalent to a talloc_steal - gives rdata to
+	 * the prs_struct state->incoming_frag.
 	 */
-	rdata_copy = (char *)memdup(rdata, rdata_len);
-	TALLOC_FREE(rdata);
-	if (tevent_req_nomem(rdata_copy, req)) {
-		return;
-	}
-	prs_give_memory(&state->incoming_frag, rdata_copy, rdata_len, true);
+	prs_give_memory(&state->incoming_frag, (char *)rdata, rdata_len, true);
+	rdata = NULL;
 
 	/* Ensure we have enough data for a pdu. */
 	subreq = get_complete_frag_send(state, state->ev, state->cli,
@@ -1597,9 +1591,10 @@ static NTSTATUS rpc_api_pipe_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
 	reply_pdu->mem_ctx = mem_ctx;
 
 	/*
-	 * Prevent state->incoming_pdu from being freed in
-	 * rpc_api_pipe_state_destructor()
+	 * Prevent state->incoming_pdu from being freed
+	 * when state is freed.
 	 */
+	talloc_steal(mem_ctx, prs_data_p(reply_pdu));
 	prs_init_empty(&state->incoming_pdu, state, UNMARSHALL);
 
 	return NT_STATUS_OK;
@@ -2458,9 +2453,10 @@ NTSTATUS rpc_api_pipe_req_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
 	reply_pdu->mem_ctx = mem_ctx;
 
 	/*
-	 * Prevent state->req_pdu from being freed in
-	 * rpc_api_pipe_req_state_destructor()
+	 * Prevent state->req_pdu from being freed
+	 * when state is freed.
 	 */
+	talloc_steal(mem_ctx, prs_data_p(reply_pdu));
 	prs_init_empty(&state->reply_pdu, state, UNMARSHALL);
 
 	return NT_STATUS_OK;
diff --git a/source3/rpc_client/ndr.c b/source3/rpc_client/ndr.c
index bbd78067bf..8e03f2e015 100644
--- a/source3/rpc_client/ndr.c
+++ b/source3/rpc_client/ndr.c
@@ -103,7 +103,6 @@ static void cli_do_rpc_ndr_done(struct tevent_req *subreq)
 
 	status = rpc_api_pipe_req_recv(subreq, state, &state->r_ps);
 	TALLOC_FREE(subreq);
-	prs_mem_free(&state->q_ps);
 	if (!NT_STATUS_IS_OK(status)) {
 		tevent_req_nterror(req, status);
 		return;
@@ -126,7 +125,6 @@ NTSTATUS cli_do_rpc_ndr_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx)
 	}
 
 	ret = prs_data_blob(&state->r_ps, &blob, talloc_tos());
-	prs_mem_free(&state->r_ps);
 	if (!ret) {
 		return NT_STATUS_NO_MEMORY;
 	}
diff --git a/source3/rpc_parse/parse_prs.c b/source3/rpc_parse/parse_prs.c
index 42e6ce1b7b..af1cc3d5b2 100644
--- a/source3/rpc_parse/parse_prs.c
+++ b/source3/rpc_parse/parse_prs.c
@@ -93,7 +93,7 @@ void prs_debug(prs_struct *ps, int depth, const char *desc, const char *fn_name)
  * Initialise an expandable parse structure.
  *
  * @param size Initial buffer size.  If >0, a new buffer will be
- * created with malloc().
+ * created with talloc().
  *
  * @return False if allocation fails, otherwise True.
  **/
@@ -112,11 +112,11 @@ bool prs_init(prs_struct *ps, uint32 size, TALLOC_CTX *ctx, bool io)
 
 	if (size != 0) {
 		ps->buffer_size = size;
-		if((ps->data_p = (char *)SMB_MALLOC((size_t)size)) == NULL) {
-			DEBUG(0,("prs_init: malloc fail for %u bytes.\n", (unsigned int)size));
+		ps->data_p = talloc_zero_size(ps->mem_ctx, size);
+		if(ps->data_p == NULL) {
+			DEBUG(0,("prs_init: talloc fail for %u bytes.\n", (unsigned int)size));
 			return False;
 		}
-		memset(ps->data_p, '\0', (size_t)size);
 		ps->is_dynamic = True; /* We own this memory. */
 	} else if (MARSHALLING(ps)) {
 		/* If size is zero and we're marshalling we should allocate memory on demand. */
@@ -130,14 +130,18 @@ bool prs_init(prs_struct *ps, uint32 size, TALLOC_CTX *ctx, bool io)
  Delete the memory in a parse structure - if we own it.
 
  NOTE: Contrary to the somewhat confusing naming, this function is not
-       intended for freeing memory allocated by prs_alloc_mem().  That memory
-       is attached to the talloc context given by ps->mem_ctx.
+       intended for freeing memory allocated by prs_alloc_mem().
+       That memory is also attached to the talloc context given by
+       ps->mem_ctx, but is only freed when that talloc context is
+       freed. prs_mem_free() is used to delete "dynamic" memory
+       allocated in marshalling/unmarshalling.
  ********************************************************************/
 
 void prs_mem_free(prs_struct *ps)
 {
-	if(ps->is_dynamic)
-		SAFE_FREE(ps->data_p);
+	if(ps->is_dynamic) {
+		TALLOC_FREE(ps->data_p);
+	}
 	ps->is_dynamic = False;
 	ps->buffer_size = 0;
 	ps->data_offset = 0;
@@ -184,11 +188,17 @@ TALLOC_CTX *prs_get_mem_context(prs_struct *ps)
 
 /*******************************************************************
  Hand some already allocated memory to a prs_struct.
+ If "is_dynamic" is true, this is a talloc_steal.
+ If "is_dynamic" is false, just make ps->data_p a pointer to
+ the unwritable memory.
  ********************************************************************/
 
 void prs_give_memory(prs_struct *ps, char *buf, uint32 size, bool is_dynamic)
 {
 	ps->is_dynamic = is_dynamic;
+	if (ps->is_dynamic && buf) {
+		talloc_steal(ps->mem_ctx, buf);
+	}
 	ps->data_p = buf;
 	ps->buffer_size = size;
 }
@@ -207,14 +217,16 @@ bool prs_set_buffer_size(prs_struct *ps, uint32 newsize)
 
 		/* newsize == 0 acts as a free and set pointer to NULL */
 		if (newsize == 0) {
-			SAFE_FREE(ps->data_p);
+			TALLOC_FREE(ps->data_p);
 		} else {
-			ps->data_p = (char *)SMB_REALLOC(ps->data_p, newsize);
+			ps->data_p = talloc_realloc(ps->mem_ctx,
+						ps->data_p,
+						char,
+						newsize);
 
 			if (ps->data_p == NULL) {
 				DEBUG(0,("prs_set_buffer_size: Realloc failure for size %u.\n",
 					(unsigned int)newsize));
-				DEBUG(0,("prs_set_buffer_size: Reason %s\n",strerror(errno)));
 				return False;
 			}
 		}
@@ -261,11 +273,11 @@ bool prs_grow(prs_struct *ps, uint32 extra_space)
 		 */
 		new_size = MAX(128, extra_space);
 
-		if((ps->data_p = (char *)SMB_MALLOC(new_size)) == NULL) {
-			DEBUG(0,("prs_grow: Malloc failure for size %u.\n", (unsigned int)new_size));
+		ps->data_p = (char *)talloc_zero_size(ps->mem_ctx, new_size);
+		if(ps->data_p == NULL) {
+			DEBUG(0,("prs_grow: talloc failure for size %u.\n", (unsigned int)new_size));
 			return False;
 		}
-		memset(ps->data_p, '\0', (size_t)new_size );
 	} else {
 		/*
 		 * If the current buffer size is bigger than the space needed,
@@ -276,7 +288,11 @@ bool prs_grow(prs_struct *ps, uint32 extra_space)
 		new_size = MAX(ps->buffer_size*2,
 			       ps->buffer_size + extra_space + 64);
 
-		if ((ps->data_p = (char *)SMB_REALLOC(ps->data_p, new_size)) == NULL) {
+		ps->data_p = talloc_realloc(ps->mem_ctx,
+						ps->data_p,
+						char,
+						new_size);
+		if (ps->data_p == NULL) {
 			DEBUG(0,("prs_grow: Realloc failure for size %u.\n",
 				(unsigned int)new_size));
 			return False;
@@ -305,7 +321,11 @@ bool prs_force_grow(prs_struct *ps, uint32 extra_space)
 		return False;
 	}
 
-	if((ps->data_p = (char *)SMB_REALLOC(ps->data_p, new_size)) == NULL) {
+	ps->data_p = (char *)talloc_realloc(ps->mem_ctx,
+					ps->data_p,
+					char,
+					new_size);
+	if(ps->data_p == NULL) {
 		DEBUG(0,("prs_force_grow: Realloc failure for size %u.\n",
 			(unsigned int)new_size));
 		return False;
diff --git a/source3/rpc_server/srv_pipe_hnd.c b/source3/rpc_server/srv_pipe_hnd.c
index 975f5b823a..a77b9eabc0 100644
--- a/source3/rpc_server/srv_pipe_hnd.c
+++ b/source3/rpc_server/srv_pipe_hnd.c
@@ -237,24 +237,29 @@ static ssize_t unmarshall_rpc_header(pipes_struct *p)
 }
 
 /****************************************************************************
- Call this to free any talloc'ed memory. Do this before and after processing
- a complete PDU.
+  Call this to free any talloc'ed memory. Do this after processing
+  a complete incoming and outgoing request (multiple incoming/outgoing
+  PDU's).
 ****************************************************************************/
 
 static void free_pipe_context(pipes_struct *p)
 {
-	if (p->mem_ctx) {
-		DEBUG(3, ("free_pipe_context: "
-			  "destroying talloc pool of size %lu\n",
-			  (unsigned long)talloc_total_size(p->mem_ctx)));
-		talloc_free_children(p->mem_ctx);
-	} else {
-		p->mem_ctx = talloc_named(p, 0, "pipe %s %p",
-				    get_pipe_name_from_syntax(talloc_tos(),
-							      &p->syntax), p);
-		if (p->mem_ctx == NULL) {
-			p->fault_state = True;
-		}
+	prs_mem_free(&p->out_data.frag);
+	prs_mem_free(&p->out_data.rdata);
+	prs_mem_free(&p->in_data.data);
+
+	DEBUG(3, ("free_pipe_context: "
+		"destroying talloc pool of size %lu\n",
+		(unsigned long)talloc_total_size(p->mem_ctx)));
+	talloc_free_children(p->mem_ctx);
+	/*
+	 * Re-initialize to set back to marshalling and set the
+	 * offset back to the start of the buffer.
+	 */
+	if(!prs_init(&p->in_data.data, 128, p->mem_ctx, MARSHALL)) {
+		DEBUG(0, ("free_pipe_context: "
+			  "rps_init failed!\n"));
+		p->fault_state = True;
 	}
 }
 
@@ -399,24 +404,10 @@ static bool process_request_pdu(pipes_struct *p, prs_struct *rpc_in_p)
 		 * Process the complete data stream here.
 		 */
 
-		free_pipe_context(p);
-
 		if(pipe_init_outgoing_data(p)) {
 			ret = api_pipe_request(p);
 		}
 
-		free_pipe_context(p);
-
-		/*
-		 * We have consumed the whole data stream. Set back to
-		 * marshalling and set the offset back to the start of
-		 * the buffer to re-use it (we could also do a prs_mem_free()
-		 * and then re_init on the next start of PDU. Not sure which
-		 * is best here.... JRA.
-		 */
-
-		prs_switch_type(&p->in_data.data, MARSHALL);
-		prs_set_offset(&p->in_data.data, 0);
 		return ret;
 	}
 
@@ -868,6 +859,16 @@ static ssize_t read_from_internal_pipe(struct pipes_struct *p, char *data,
 		p->out_data.current_pdu_sent = 0;
 		prs_mem_free(&p->out_data.frag);
 	}
+
+	if(p->out_data.data_sent_length >= prs_offset(&p->out_data.rdata)) {
+		/*
+		 * We're completely finished with both outgoing and
+		 * incoming data streams. It's safe to free all temporary
+		 * data from this request.
+		 */
+		free_pipe_context(p);
+	}
+
 	return data_returned;
 }