Message-ID: <1967443.iUBOHzf2Bp@harkonnen>
Date:   Tue, 25 Jul 2017 16:21:11 +0200
From:   Federico Vaga <federico.vaga@...a.pv.it>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] trace-cmd: use asprintf when possible

Hi Steven,

I found some free time and unfortunately I can't enjoy the sun, so here I am 
on this patch.
Before submitting the V2, one comment (inline)

On Monday, July 10, 2017 2:08:41 AM CEST Federico Vaga wrote:
> 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@...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.
> 
> 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,

While I was fixing my patch according to what we said last time, I think I 
recall what was my true original meaning of  "/* FIXME is it ok ? */". (What I 
wrote last time is still valid anyway)

The questions comes by the fact that this line:

*p = '\0'; /* FIXME is it ok ? */

changes the input parameter by cutting it (it does what dirname() does).
So, "is it ok (to cut the input string)?". According to the internal usage, 
when a function uses `create_event()`, it passes a generated string that then 
is not used anymore. So, nobody cares if this string has been manipulated by 
create_event().

I think that this should not happen. So I will propose a patch V2 where I use 
`dirname()` as suggested but on local duplicate using `strdup()`. This 
guarantee (even if it is not necessary) that the input string does not change.


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

I forget to answer to this point last time.

The function `append_file()` is implemented twice in trace-stat.c and trace-
util.c

I noticed that those two files are included in different binaries (trace-cmd 
and the libraries). I just put a note because instead of having multiple 
implementation we can have just one in a file that gets included where is 
needed. Of course, if it is just for such a simple function it does not make 
much sense right now. But if we can group all the internal helpers I believe 
is better.

I will remove the fixme from the V2 patch

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

Powered by blists - more mailing lists