Re: [PATCH 24/34] kernelshark: Fix potential memory leaks in libkshark-tepdata

[Yordan Karadzhov <y.karadz@xxxxxxxxx>]

On 1/14/24 19:17, Benjamin ROBIN wrote:
> - In tepdata_get_field_names(), buffer was never free on error
> - In kshark_tep_open_buffer(), names were never free if
>    kshark_get_data_stream() failed
> - In kshark_tep_open_buffer(), prevent any double free error with
>    "name" and "file" fields of buffer_stream
> - In kshark_tep_init_all_buffers(), return failure code if failed to
>    copy "name" and "file" fields of buffer_stream
>
> Signed-off-by: Benjamin ROBIN <dev@xxxxxxxxxxxxx>
> ---
>   src/libkshark-tepdata.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
> index 2d0fcb0..d15c155 100644
> --- a/src/libkshark-tepdata.c
> +++ b/src/libkshark-tepdata.c
> @@ -949,6 +949,7 @@ static int tepdata_get_field_names(struct kshark_data_stream *stream,
>   	for (i = 0; i < nr_fields; ++i)
>   		free(buffer[i]);
>   
> +	free(buffer);
>   	return -EFAULT;
>   }
>   
> @@ -1424,8 +1425,10 @@ int kshark_tep_open_buffer(struct kshark_context *kshark_ctx, int sd,
>   
>   	sd_buffer = kshark_add_stream(kshark_ctx);
>   	buffer_stream = kshark_get_data_stream(kshark_ctx, sd_buffer);
> -	if (!buffer_stream)
> -		return -EFAULT;
> +	if (!buffer_stream) {
> +		ret = -EFAULT;
> +		goto fail;
> +	}
>   
>   	for (i = 0; i < n_buffers; ++i) {
>   		if (strcmp(buffer_name, names[i]) == 0) {
> @@ -1438,7 +1441,8 @@ int kshark_tep_open_buffer(struct kshark_context *kshark_ctx, int sd,
>   			if (!buffer_stream->name || !buffer_stream->file) {
>   				free(buffer_stream->name);
>   				free(buffer_stream->file);
> -
> +				buffer_stream->name = NULL;
> +				buffer_stream->file = NULL;
>   				ret = -ENOMEM;
>   				break;
>   			}
> @@ -1449,6 +1453,7 @@ int kshark_tep_open_buffer(struct kshark_context *kshark_ctx, int sd,
>   		}
>   	}
>   
> +fail:

This is not a true 'fail' because the code below gets executed even
if everything is fine. Perhaps you can use "free" or "end" or something
similar?

Thanks,
Y.

>   	for (i = 0; i < n_buffers; ++i)
>   		free(names[i]);
>   	free(names);
> @@ -1500,8 +1505,9 @@ int kshark_tep_init_all_buffers(struct kshark_context *kshark_ctx,
>   		if (!buffer_stream->name || !buffer_stream->file) {
>   			free(buffer_stream->name);
>   			free(buffer_stream->file);
> -			ret = -ENOMEM;
> -			break;
> +			buffer_stream->name = NULL;
> +			buffer_stream->file = NULL;
> +			return -ENOMEM;
>   		}
>   
>   		ret = kshark_tep_stream_init(buffer_stream, buffer_input);