Re: [PATCH v2 2/3] kernel-shark: Fix potential memory leak in libkshark-collection

"Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> · Thu, 28 Nov 2019 11:25:33 +0200

On 27.11.19 г. 20:54 ч., Steven Rostedt wrote:

On Wed, 23 Oct 2019 15:21:44 +0300
"Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> wrote:


When searching for the entry, do not loop over the original list of
requests. Use a copy instead. If we loop over the original list and
no entry is found in the first element of the list, later the memory
used for this first element will leak.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@xxxxxxxxx>
---
  kernel-shark/src/libkshark-collection.c | 14 ++++++--------
  1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel-shark/src/libkshark-collection.c b/kernel-shark/src/libkshark-collection.c
index 02a014e..95fdbab 100644
--- a/kernel-shark/src/libkshark-collection.c
+++ b/kernel-shark/src/libkshark-collection.c
@@ -622,6 +622,7 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
  				  ssize_t *index)
  {
  	const struct kshark_entry *entry = NULL;
+	struct kshark_entry_request *list;


Hi Yordan,

I was looking at this patch in more detail, and I'm thinking that we
don't need to pass in the address of the req pointer, but just the req
pointer itself. The only place that I see the req pointer being
modified is the failure case in map_collection_request_init() where it
does:

	kshark_free_entry_request(*req);
	*req = NULL;

But all callers do that free anyway.




Yes, because the caller is expected to do 

kshark_free_entry_request(*req) at the end, here we have to set the 

original pointer to NULL. Otherwise we will get double free error.

I think this is what I have been trying to fix, when I introduced the 

memory leak.



And yes, I agree with you that carrying the address of the pointer 

through all these functions is a bit ugly.


Thanks!
Yordan


Maybe I'm missing something, but why are we passing in the pointer to
the pointer of req, and not just the req pointer itself? I don't see a
need to modify the pointer.

Before this patch, *req is modified, but after this patch, it is not.
If you pass in just "struct kshark_entry_request *req" then you don't
even need to have the "list" variable, you could just use "req" because
that would be a copy of the pointer.

-- Steve




  	int req_count;

  

  	/*

@@ -638,12 +639,10 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
  	 * Loop over the list of redefined requests and search until you find
  	 * the first matching entry.
  	 */
-	while (*req) {
-		entry = kshark_get_entry_front(*req, data, index);
+	for (list = *req; list; list = list->next) {
+		entry = kshark_get_entry_front(list, data, index);
  		if (entry)
  			break;
-
-		*req = (*req)->next;
  	}

  

  	return entry;

@@ -680,6 +679,7 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req,
  				 ssize_t *index)
  {
  	const struct kshark_entry *entry = NULL;
+	struct kshark_entry_request *list;
  	int req_count;

  

  	/*

@@ -695,12 +695,10 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req,
  	 * Loop over the list of redefined requests and search until you find
  	 * the first matching entry.
  	 */
-	while (*req) {
-		entry = kshark_get_entry_back(*req, data, index);
+	for (list = *req; list; list = list->next) {
+		entry = kshark_get_entry_back(list, data, index);
  		if (entry)
  			break;
-
-		*req = (*req)->next;
  	}

  

  	return entry;





