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

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 23 Oct 2019 15:21:44 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> 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@gmail.com>
> ---
>  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.

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;