Re: [PATCH 20/34] kernelshark: Fix potential memory leaks in libkshark-configio

[Yordan Karadzhov <y.karadz@gmail.com>]

On 1/14/24 19:17, Benjamin ROBIN wrote:
> Allocate a new kshark_config_doc only if the format is supported.
> 
> Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr>
> ---
>   src/libkshark-configio.c | 74 ++++++++++++++++++++++------------------
>   1 file changed, 41 insertions(+), 33 deletions(-)
> 
> diff --git a/src/libkshark-configio.c b/src/libkshark-configio.c
> index 9a1ba60..853e056 100644
> --- a/src/libkshark-configio.c
> +++ b/src/libkshark-configio.c
> @@ -675,23 +675,25 @@ struct kshark_config_doc *
>   kshark_export_plugin_file(struct kshark_plugin_list *plugin,
>   			  enum kshark_config_formats format)
>   {
> -	/*  Create a new Configuration document. */
> -	struct kshark_config_doc *conf =
> -		kshark_config_new("kshark.config.library", format);
> -
> -	if (!conf)
> -		return NULL;
> +	struct kshark_config_doc *conf;
>   
>   	switch (format) {
>   	case KS_CONFIG_JSON:
> -		kshark_plugin_to_json(plugin, conf->conf_doc);
> -		return conf;
> +		break;
>   
>   	default:
>   		fprintf(stderr, "Document format %d not supported\n",
> -			conf->format);
> +			format);
>   		return NULL;
>   	}
> +
> +	/*  Create a new Configuration document. */
> +	conf = kshark_config_new("kshark.config.library", format);
> +	if (!conf)
> +		return NULL;
> +
> +	kshark_plugin_to_json(plugin, conf->conf_doc);
> +	return conf;
>   }
>   
>   static bool kshark_all_plugins_to_json(struct kshark_context *kshark_ctx,
> @@ -739,22 +741,24 @@ struct kshark_config_doc *
>   kshark_export_all_plugins(struct kshark_context *kshark_ctx,
>   			  enum kshark_config_formats format)
>   {
> -	struct kshark_config_doc *conf =
> -		kshark_config_new("kshark.config.plugins", KS_CONFIG_JSON);
> -
> -	if (!conf)
> -		return NULL;
> +	struct kshark_config_doc *conf;
>   
>   	switch (format) {
>   	case KS_CONFIG_JSON:
> -		kshark_all_plugins_to_json(kshark_ctx, conf->conf_doc);
> -		return conf;

I agree that this code is a bit confusing. Long time ago we had the
idea that we will support not only 'json' but also other formats. And
that we will have other 'kshark_plugin_to_xxx()' APIs. This is the
reason to have this 'switch'.

Please keep the code as it is and just call kshark_free_config_doc()
in the default (error) case.

> +		break;
>   
>   	default:
>   		fprintf(stderr, "Document format %d not supported\n",
> -			conf->format);
> +			format);
>   		return NULL;
>   	}
> +
> +	conf = kshark_config_new("kshark.config.plugins", format);
> +	if (!conf)
> +		return NULL;
> +
> +	kshark_all_plugins_to_json(kshark_ctx, conf->conf_doc);
> +	return conf;
>   }
>   
>   static bool kshark_plugin_from_json(struct kshark_context *kshark_ctx,
> @@ -867,22 +871,24 @@ struct kshark_config_doc *
>   kshark_export_stream_plugins(struct kshark_data_stream *stream,
>   			     enum kshark_config_formats format)
>   {
> -	struct kshark_config_doc *conf =
> -		kshark_config_new("kshark.config.plugins", KS_CONFIG_JSON);
> -
> -	if (!conf)
> -		return NULL;
> +	struct kshark_config_doc *conf;
>   
>   	switch (format) {
>   	case KS_CONFIG_JSON:
> -		kshark_stream_plugins_to_json(stream, conf->conf_doc);
> -		return conf;

Same comment applies here.
Thanks!
Y.

> +		break;
>   
>   	default:
>   		fprintf(stderr, "Document format %d not supported\n",
> -			conf->format);
> +			format);
>   		return NULL;
>   	}
> +
> +	conf = kshark_config_new("kshark.config.plugins", KS_CONFIG_JSON);
> +	if (!conf)
> +		return NULL;
> +
> +	kshark_stream_plugins_to_json(stream, conf->conf_doc);
> +	return conf;
>   }
>   
>   static bool kshark_stream_plugins_from_json(struct kshark_context *kshark_ctx,
> @@ -1019,23 +1025,25 @@ struct kshark_config_doc *
>   kshark_export_model(struct kshark_trace_histo *histo,
>   		    enum kshark_config_formats format)
>   {
> -	/*  Create a new Configuration document. */
> -	struct kshark_config_doc *conf =
> -		kshark_config_new("kshark.config.model", format);
> -
> -	if (!conf)
> -		return NULL;
> +	struct kshark_config_doc *conf;
>   
>   	switch (format) {
>   	case KS_CONFIG_JSON:
> -		kshark_model_to_json(histo, conf->conf_doc);
> -		return conf;
> +		break;
>   
>   	default:
>   		fprintf(stderr, "Document format %d not supported\n",
>   			format);
>   		return NULL;
>   	}
> +
> +	/*  Create a new Configuration document. */
> +	conf = kshark_config_new("kshark.config.model", format);
> +	if (!conf)
> +		return NULL;
> +
> +	kshark_model_to_json(histo, conf->conf_doc);
> +	return conf;
>   }
>   
>   static bool kshark_model_from_json(struct kshark_trace_histo *histo,