Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Federico Vaga]

On Friday, July 7, 2017 12:25:32 AM CEST Steven Rostedt wrote:
> On Mon,  5 Jun 2017 11:31:17 +0200
> Federico Vaga <federico.vaga@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 
> >  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.

At the beginning the logic was not clear to me, in particular "why is it doing 
this?", then I understood by having a look at the usage of `create_event()` 
but I forget to remove the FIXME.


But still, it is not immediately obvious why we need this without reading how 
the function has been used.

Answer to the question:
we need it because when we call `create_event()` we pass the path to the 
filter file, and not the path to the event directory.


In my opinion we should pass the path to the event directory, and from this we 
can build the event_list's paths. To me, it sounds more correct for a function 
named `create_event()`, rather than:
- taking a path to a specific event file,
- deduce the event directory,
- build the path for the other event files,

> 
> > +	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.

Yes, you are right. 

> 
> > +	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".

yes

> >  	/* 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?

I can send an updated patch, but I do not know when (weeks). It is a super 
easy patch but I'm really busy and tired at the moment. Sorry :/

If you have time and you want to do it, please go ahead. Otherwise I will do 
it when I will have free time :)

-- 
Federico Vaga
http://www.federicovaga.it/