Message-ID: <20170706182532.212cba4f@gandalf.local.home>
Date:   Thu, 6 Jul 2017 18:25:32 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Federico Vaga <federico.vaga@...a.pv.it>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] trace-cmd: use asprintf when possible

On Mon,  5 Jun 2017 11:31:17 +0200
Federico Vaga <federico.vaga@...a.pv.it> wrote:

Hi Federico,

I finally got around to looking at these. Sorry for the really slow
reply, but I had a bunch of kernel work I needed to get done before
digging again into trace-cmd.


> It makes the code clearer and less error prone.
> 
> clearer:
> - less code
> - the code is now using the same format to create strings dynamically
> 
> less error prone:
> - no magic number +2 +9 +5 to compute the size
> - no copy&paste of the strings to compute the size and to concatenate

I like this!

> 
> The function `asprintf` is not POSIX standard but the program
> was already using it. Later it can be decided to use only POSIX
> functions, then we can easly replace all the `asprintf(3)` with a local
> implementation of that function.

Yes, if we decide not to use GNU specific code, then we can implement
our own version.

> 
> Signed-off-by: Federico Vaga <federico.vaga@...a.pv.it>
> ---
>  event-plugin.c   | 24 +++++++++-------------
>  parse-filter.c   | 11 ++++------
>  trace-list.c     |  8 ++++----
>  trace-output.c   |  6 +++---
>  trace-record.c   | 56 +++++++++++++++++++++------------------------------
>  trace-recorder.c | 11 +++++-----
>  trace-show.c     |  8 ++++----
>  trace-split.c    |  7 ++++---
>  trace-stat.c     |  7 ++++---
>  trace-util.c     | 61 +++++++++++++++++++++++---------------------------------
>  10 files changed, 85 insertions(+), 114 deletions(-)

[...]

> @@ -2237,11 +2233,10 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
>  	for (p = path + strlen(path) - 1; p > path; p--)
>  		if (*p == '/')
>  			break;
> -	*p = '\0';
> -	p = malloc(strlen(path) + strlen("/enable") + 1);
> -	if (!p)
> +	*p = '\0'; /* FIXME is it ok ? */

I'm going to remove the comment. If you look at the code above it, You
will see that 'p' is used to find the last instance of '/' in path.
Then the *p = '\0' is used to remove it.

> +	ret = asprintf(&p, "%s/enable", path);
> +	if (ret < 0)
>  		die("Failed to allocate enable path for %s", path);
> -	sprintf(p, "%s/enable", path);
>  	ret = stat(p, &st);
>  	if (ret >= 0)
>  		event->enable_file = p;
> @@ -2249,10 +2244,9 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
>  		free(p);
>  
>  	if (event->trigger) {
> -		p = malloc(strlen(path) + strlen("/trigger") + 1);
> -		if (!p)
> +		ret = asprintf(&p, "%s/trigger", path);
> +		if (ret < 0)
>  			die("Failed to allocate trigger path for %s", path);
> -		sprintf(p, "%s/trigger", path);
>  		ret = stat(p, &st);
>  		if (ret > 0)
>  			die("trigger specified but not supported by this kernel");
> @@ -2268,17 +2262,16 @@ static void make_sched_event(struct buffer_instance *instance,
>  {
>  	char *path;
>  	char *p;
> +	int ret;
>  
>  	/* Do nothing if the event already exists */
>  	if (*event)
>  		return;
>  
> -	path = malloc(strlen(sched->filter_file) + strlen(sched_path) + 2);
> -	if (!path)
> +	ret = asprintf(&path, "%s", sched->filter_file);

Now this part is not correct. It is actually buggy. If you noticed the
malloc, it allocates strlen(sched->filter_file) + strlen(sched_path) + 2.
Which is probably a little more than needed.


> +	if (ret < 0)
>  		die("Failed to allocate path for %s", sched_path);
>  
> -	sprintf(path, "%s", sched->filter_file);
> -

Here I copy in the sched->filter_file which is the path I want, but I
don't want the "/filter" part. So it is cut off below and the
sched_path is copied in.

Hmm, what would be better is to:

	asprintf(path, "%s/%s", dirname(sched->filter_file), sched_path);

And remove all this open coded stuff. The same can probably be done
above where you had the "fixme".


>  	/* Remove the /filter from filter file */
>  	p = path + strlen(path) - strlen("filter");
>  	sprintf(p, "%s/filter", sched_path);
> @@ -2310,10 +2303,9 @@ static int expand_event_files(struct buffer_instance *instance,
>  	int ret;
>  	int i;
>  
> -	p = malloc(strlen(file) + strlen("events//filter") + 1);
> -	if (!p)
> +	ret = asprintf(&p, "events/%s/filter", file);
> +	if (ret < 0)
>  		die("Failed to allocate event filter path for %s", file);
> -	sprintf(p, "events/%s/filter", file);
>  
>  	path = get_instance_file(instance, p);
>  
> @@ -3956,6 +3948,8 @@ static int recording_all_events(void)
>  
>  static void add_trigger(struct event_list *event, const char *trigger)
>  {
> +	int ret;
> +
>  	if (event->trigger) {
>  		event->trigger = realloc(event->trigger,
>  					 strlen(event->trigger) + strlen("\n") +
> @@ -3963,10 +3957,9 @@ static void add_trigger(struct event_list *event, const char *trigger)
>  		strcat(event->trigger, "\n");
>  		strcat(event->trigger, trigger);
>  	} else {
> -		event->trigger = malloc(strlen(trigger) + 1);
> -		if (!event->trigger)
> +		ret = asprintf(&event->trigger, "%s", trigger);
> +		if (ret < 0)
>  			die("Failed to allocate event trigger");
> -		sprintf(event->trigger, "%s", trigger);
>  	}
>  }
>  
> @@ -4357,7 +4350,7 @@ void trace_record (int argc, char **argv)
>  		usage(argv);
>  
>  	for (;;) {
> -		int option_index = 0;
> +		int option_index = 0, ret;
>  		const char *opts;
>  		static struct option long_options[] = {
>  			{"date", no_argument, NULL, OPT_date},
> @@ -4420,12 +4413,9 @@ void trace_record (int argc, char **argv)
>  				strcat(last_event->filter, optarg);
>  				strcat(last_event->filter, ")");
>  			} else {
> -				last_event->filter =
> -					malloc(strlen(optarg) +
> -					       strlen("()") + 1);
> -				if (!last_event->filter)
> +				ret = asprintf(&last_event->filter, "(%s)", optarg);
> +				if (ret < 0)
>  					die("Failed to allocate filter %s", optarg);
> -				sprintf(last_event->filter, "(%s)", optarg);
>  			}
>  			break;
>  
> diff --git a/trace-recorder.c b/trace-recorder.c
> index 1b9d364..85150fd 100644
> --- a/trace-recorder.c
> +++ b/trace-recorder.c
> @@ -156,14 +156,13 @@ tracecmd_create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags,
>  	recorder->fd1 = fd;
>  	recorder->fd2 = fd2;
>  
> -	path = malloc(strlen(buffer) + 40);
> -	if (!path)
> -		goto out_free;
> -
>  	if (flags & TRACECMD_RECORD_SNAPSHOT)
> -		sprintf(path, "%s/per_cpu/cpu%d/snapshot_raw", buffer, cpu);
> +		ret = asprintf(&path, "%s/per_cpu/cpu%d/snapshot_raw", buffer, cpu);
>  	else
> -		sprintf(path, "%s/per_cpu/cpu%d/trace_pipe_raw", buffer, cpu);
> +		ret = asprintf(&path, "%s/per_cpu/cpu%d/trace_pipe_raw", buffer, cpu);
> +	if (ret < 0)
> +		goto out_free;
> +
>  	recorder->trace_fd = open(path, O_RDONLY);
>  	if (recorder->trace_fd < 0)
>  		goto out_free;
> diff --git a/trace-show.c b/trace-show.c
> index 14b786c..f13db31 100644
> --- a/trace-show.c
> +++ b/trace-show.c
> @@ -154,11 +154,11 @@ void trace_show(int argc, char **argv)
>  	}
>  
>  	if (buffer) {
> -		path = malloc(strlen(buffer) + strlen("instances//") +
> -			      strlen(file) + 1);
> -		if (!path)
> +		int ret;
> +
> +		ret = asprintf(&path, "instances/%s/%s", buffer, file);
> +		if (ret < 0)
>  			die("Failed to allocate instance path %s", file);
> -		sprintf(path, "instances/%s/%s", buffer, file);
>  		file = path;
>  	}
>  
> diff --git a/trace-split.c b/trace-split.c
> index 87d43ad..5e4ac68 100644
> --- a/trace-split.c
> +++ b/trace-split.c
> @@ -369,10 +369,11 @@ static double parse_file(struct tracecmd_input *handle,
>  		die("Failed to allocate cpu_data for %d cpus", cpus);
>  
>  	for (cpu = 0; cpu < cpus; cpu++) {
> -		file = malloc(strlen(output_file) + 50);
> -		if (!file)
> +		int ret;
> +
> +		ret = asprintf(&file, "%s/.tmp.%s.%d", dir, base, cpu);
> +		if (ret < 0)
>  			die("Failed to allocate file for %s %s %d", dir, base, cpu);
> -		sprintf(file, "%s/.tmp.%s.%d", dir, base, cpu);
>  		fd = open(file, O_WRONLY | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
>  		cpu_data[cpu].cpu = cpu;
>  		cpu_data[cpu].fd = fd;
> diff --git a/trace-stat.c b/trace-stat.c
> index adbf3c3..778c199 100644
> --- a/trace-stat.c
> +++ b/trace-stat.c
> @@ -70,15 +70,16 @@ char *strstrip(char *str)
>  	return s;
>  }
>  
> +/* FIXME repeated */

What do you mean by that?

>  char *append_file(const char *dir, const char *name)
>  {
>  	char *file;
> +	int ret;
>  
> -	file = malloc(strlen(dir) + strlen(name) + 2);
> -	if (!file)
> +	ret = asprintf(&file, "%s/%s", dir, name);
> +	if (ret < 0)
>  		die("Failed to allocate %s/%s", dir, name);
>  
> -	sprintf(file, "%s/%s", dir, name);
>  	return file;
>  }
>  
> diff --git a/trace-util.c b/trace-util.c
> index fbf8cea..29a7079 100644
> --- a/trace-util.c
> +++ b/trace-util.c
> @@ -88,14 +88,15 @@ char **trace_util_list_plugin_options(void)
>  	for (reg = registered_options; reg; reg = reg->next) {
>  		for (op = reg->options; op->name; op++) {
>  			char *alias = op->plugin_alias ? op->plugin_alias : op->file;
> +			int ret;
>  
> -			name = malloc(strlen(op->name) + strlen(alias) + 2);
> -			if (!name) {
> +			ret = asprintf(&name, "%s:%s", alias, op->name);
> +			if (ret < 0) {
>  				warning("Failed to allocate plugin option %s:%s",
>  					alias, op->name);
>  				break;
>  			}
> -			sprintf(name, "%s:%s", alias, op->name);
> +
>  			list = realloc(list, count + 2);
>  			if (!list) {
>  				warning("Failed to allocate plugin list for %s", name);
> @@ -617,14 +618,10 @@ static int load_plugin(struct pevent *pevent, const char *path,
>  	void *handle;
>  	int ret;
>  
> -	plugin = malloc(strlen(path) + strlen(file) + 2);
> -	if (!plugin)
> +	ret = asprintf(&plugin, "%s/%s", path, file);
> +	if (ret < 0)
>  		return -ENOMEM;
>  
> -	strcpy(plugin, path);
> -	strcat(plugin, "/");
> -	strcat(plugin, file);
> -
>  	handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
>  	if (!handle) {
>  		warning("cound not load plugin '%s'\n%s\n",
> @@ -710,7 +707,7 @@ char *tracecmd_find_tracing_dir(void)
>  	char type[100];
>  	int use_debug = 0;
>  	FILE *fp;
> -	
> +
>  	if ((fp = fopen("/proc/mounts","r")) == NULL) {
>  		warning("Can't open /proc/mounts for read");
>  		return NULL;
> @@ -752,16 +749,16 @@ char *tracecmd_find_tracing_dir(void)
>  	free(debug_str);
>  
>  	if (use_debug) {
> -		tracing_dir = malloc(strlen(fspath) + 9);
> -		if (!tracing_dir)
> -			return NULL;
> +		int ret;
>  
> -		sprintf(tracing_dir, "%s/tracing", fspath);
> +		ret = asprintf(&tracing_dir, "%s/tracing", fspath);
> +		if (ret < 0)
> +			return NULL;
>  	} else {
>  		tracing_dir = strdup(fspath);
>  		if (!tracing_dir)
>  			return NULL;
> -	}		
> +	}
>  
>  	return tracing_dir;
>  }
> @@ -780,13 +777,11 @@ const char *tracecmd_get_tracing_dir(void)
>  static char *append_file(const char *dir, const char *name)
>  {
>  	char *file;
> +	int ret;
>  
> -	file = malloc(strlen(dir) + strlen(name) + 2);
> -	if (!file)
> -		return NULL;
> +	ret = asprintf(&file, "%s/%s", dir, name);
>  
> -	sprintf(file, "%s/%s", dir, name);
> -	return file;
> +	return ret < 0 ? NULL : file;
>  }
>  
>  /**
> @@ -1392,7 +1387,8 @@ int trace_util_load_plugins(struct pevent *pevent, const char *suffix,
>  {
>  	char *home;
>  	char *path;
> -        char *envdir;
> +	char *envdir;
> +	int ret;
>  
>  	if (tracecmd_disable_plugins)
>  		return -EBUSY;
> @@ -1415,14 +1411,10 @@ int trace_util_load_plugins(struct pevent *pevent, const char *suffix,
>  	if (!home)
>  		return -EINVAL;
>  
> -	path = malloc(strlen(home) + strlen(LOCAL_PLUGIN_DIR) + 2);
> -	if (!path)
> +	ret = asprintf(&path, "%s/%s", home, LOCAL_PLUGIN_DIR);
> +	if (ret < 0)
>  		return -ENOMEM;
>  
> -	strcpy(path, home);
> -	strcat(path, "/");
> -	strcat(path, LOCAL_PLUGIN_DIR);
> -
>  	trace_util_load_plugins_dir(pevent, suffix, path, load_plugin, data);
>  
>  	free(path);
> @@ -1509,15 +1501,12 @@ static int read_options(struct pevent *pevent, const char *path,
>  	int unload = 0;
>  	char *plugin;
>  	void *handle;
> +	int ret;
>  
> -	plugin = malloc(strlen(path) + strlen(file) + 2);
> -	if (!plugin)
> +	ret = asprintf(&plugin, "%s/%s", path, file);
> +	if (ret < 0)
>  		return -ENOMEM;
>  
> -	strcpy(plugin, path);
> -	strcat(plugin, "/");
> -	strcat(plugin, file);
> -
>  	handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
>  	if (!handle) {
>  		warning("cound not load plugin '%s'\n%s\n",
> @@ -1606,6 +1595,7 @@ char *tracecmd_get_tracing_file(const char *name)
>  {
>  	static const char *tracing;
>  	char *file;
> +	int ret;
>  
>  	if (!tracing) {
>  		tracing = tracecmd_find_tracing_dir();
> @@ -1613,11 +1603,10 @@ char *tracecmd_get_tracing_file(const char *name)
>  			return NULL;
>  	}
>  
> -	file = malloc(strlen(tracing) + strlen(name) + 2);
> -	if (!file)
> +	ret = asprintf(&file, "%s/%s", tracing, name);
> +	if (ret < 0)
>  		return NULL;
>  
> -	sprintf(file, "%s/%s", tracing, name);
>  	return file;
>  }
>  


The rest looks good. Do you want to send an updated patch, or do you
want me to fix the above?

-- Steve

Powered by blists - more mailing lists