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

From: Federico Vaga
Date: Wed Aug 02 2017 - 12:23:21 EST

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

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.

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 | 93 +++++++++++++++++++++++++++++---------------------------
trace-recorder.c | 11 +++----
trace-show.c | 8 ++---
trace-split.c | 7 +++--
trace-stat.c | 7 +++--
trace-util.c | 62 ++++++++++++++++---------------------
10 files changed, 112 insertions(+), 125 deletions(-)

diff --git a/event-plugin.c b/event-plugin.c
index a16756a..d542cb6 100644
--- a/event-plugin.c
+++ b/event-plugin.c
@@ -120,12 +120,12 @@ char **traceevent_plugin_list_options(void)
for (op = reg->options; op->name; op++) {
char *alias = op->plugin_alias ? op->plugin_alias : op->file;
char **temp = list;
+ int ret;

- name = malloc(strlen(op->name) + strlen(alias) + 2);
- if (!name)
+ ret = asprintf(&name, "%s:%s", alias, op->name);
+ if (ret < 0)
goto err;

- sprintf(name, "%s:%s", alias, op->name);
list = realloc(list, count + 2);
if (!list) {
list = temp;
@@ -290,17 +290,14 @@ load_plugin(struct pevent *pevent, const char *path,
const char *alias;
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) {
warning("could not allocate plugin memory\n");
return;
}

- strcpy(plugin, path);
- strcat(plugin, "/");
- strcat(plugin, file);
-
handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
if (!handle) {
warning("could not load plugin '%s'\n%s\n",
@@ -391,6 +388,7 @@ load_plugins(struct pevent *pevent, const char *suffix,
char *home;
char *path;
char *envdir;
+ int ret;

if (pevent->flags & PEVENT_DISABLE_PLUGINS)
return;
@@ -421,16 +419,12 @@ load_plugins(struct pevent *pevent, const char *suffix,
if (!home)
return;

- path = malloc(strlen(home) + strlen(LOCAL_PLUGIN_DIR) + 2);
- if (!path) {
+ ret = asprintf(&path, "%s/%s", home, LOCAL_PLUGIN_DIR);
+ if (ret < 0) {
warning("could not allocate plugin memory\n");
return;
}

- strcpy(path, home);
- strcat(path, "/");
- strcat(path, LOCAL_PLUGIN_DIR);
-
load_plugins_dir(pevent, suffix, path, load_plugin, data);

free(path);
diff --git a/parse-filter.c b/parse-filter.c
index 8e302c4..ebf2a95 100644
--- a/parse-filter.c
+++ b/parse-filter.c
@@ -287,12 +287,10 @@ find_event(struct pevent *pevent, struct event_list **events,
sys_name = NULL;
}

- reg = malloc(strlen(event_name) + 3);
- if (reg == NULL)
+ ret = asprintf(&reg, "^%s$", event_name);
+ if (ret < 0)
return PEVENT_ERRNO__MEM_ALLOC_FAILED;

- sprintf(reg, "^%s$", event_name);
-
ret = regcomp(&ereg, reg, REG_ICASE|REG_NOSUB);
free(reg);

@@ -300,13 +298,12 @@ find_event(struct pevent *pevent, struct event_list **events,
return PEVENT_ERRNO__INVALID_EVENT_NAME;

if (sys_name) {
- reg = malloc(strlen(sys_name) + 3);
- if (reg == NULL) {
+ ret = asprintf(&reg, "^%s$", sys_name);
+ if (ret < 0) {
regfree(&ereg);
return PEVENT_ERRNO__MEM_ALLOC_FAILED;
}

- sprintf(reg, "^%s$", sys_name);
ret = regcomp(&sreg, reg, REG_ICASE|REG_NOSUB);
free(reg);
if (ret) {
diff --git a/trace-list.c b/trace-list.c
index 293635f..32b19bc 100644
--- a/trace-list.c
+++ b/trace-list.c
@@ -133,6 +133,7 @@ static char *get_event_file(const char *type, char *buf, int len)
char *event;
char *path;
char *file;
+ int ret;

if (buf[len-1] == '\n')
buf[len-1] = '\0';
@@ -146,11 +147,10 @@ static char *get_event_file(const char *type, char *buf, int len)
die("no event found in %s\n", buf);

path = tracecmd_get_tracing_file("events");
- file = malloc(strlen(path) + strlen(system) + strlen(event) +
- strlen(type) + strlen("///") + 1);
- if (!file)
+ ret = asprintf(&file, "%s/%s/%s/%s", path, system, event, type);
+ if (ret < 0)
die("Failed to allocate event file %s %s", system, event);
- sprintf(file, "%s/%s/%s/%s", path, system, event, type);
+
tracecmd_put_tracing_file(path);

return file;
diff --git a/trace-output.c b/trace-output.c
index bfe6331..26ed560 100644
--- a/trace-output.c
+++ b/trace-output.c
@@ -234,16 +234,16 @@ static char *get_tracing_file(struct tracecmd_output *handle, const char *name)
{
const char *tracing;
char *file;
+ int ret;

tracing = find_tracing_dir(handle);
if (!tracing)
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;
}

diff --git a/trace-record.c b/trace-record.c
index 020a373..748dd46 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -45,6 +45,7 @@
#include <glob.h>
#include <errno.h>
#include <limits.h>
+#include <libgen.h>

#include "trace-local.h"
#include "trace-msg.h"
@@ -723,14 +724,12 @@ get_instance_file(struct buffer_instance *instance, const char *file)
{
char *buf;
char *path;
+ int ret;

if (instance->name) {
- buf = malloc(strlen(instance->name) +
- strlen(file) + strlen("instances//") + 1);
- if (!buf)
+ ret = asprintf(&buf, "instances/%s/%s", instance->name, file);
+ if (ret < 0)
die("Failed to allocate name for %s/%s", instance->name, file);
- sprintf(buf, "instances/%s/%s", instance->name, file);
-
path = tracecmd_get_tracing_file(buf);
free(buf);
} else
@@ -744,17 +743,15 @@ get_instance_dir(struct buffer_instance *instance)
{
char *buf;
char *path;
+ int ret;

/* only works for instances */
if (!instance->name)
return NULL;

- buf = malloc(strlen(instance->name) +
- strlen("instances/") + 1);
- if (!buf)
+ ret = asprintf(&buf, "instances/%s", instance->name);
+ if (ret < 0)
die("Failed to allocate for instance %s", instance->name);
- sprintf(buf, "instances/%s", instance->name);
-
path = tracecmd_get_tracing_file(buf);
free(buf);

@@ -2215,14 +2212,28 @@ static void set_max_graph_depth(struct buffer_instance *instance, char *max_grap
die("could not write to max_graph_depth");
}

+
+/**
+ * create_event - create and event descriptor
+ * @instance: instance to use
+ * @path: path to event attribute
+ * @old_event: event descriptor to use as base
+ *
+ * NOTE: the function purpose is to create a data structure to describe
+ * an ftrace event. During the process it becomes handy to change the
+ * string `path`. So, do not rely on the content of `path` after you
+ * invoke this function.
+ */
static struct event_list *
create_event(struct buffer_instance *instance, char *path, struct event_list *old_event)
{
struct event_list *event;
struct stat st;
- char *p;
+ char *p, *path_dirname;
int ret;

+ path_dirname = dirname(path);
+
event = malloc(sizeof(*event));
if (!event)
die("Failed to allocate event");
@@ -2234,14 +2245,10 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
if (!event->filter_file)
die("malloc filter file");
}
- for (p = path + strlen(path) - 1; p > path; p--)
- if (*p == '/')
- break;
- *p = '\0';
- p = malloc(strlen(path) + strlen("/enable") + 1);
- if (!p)
- die("Failed to allocate enable path for %s", path);
- sprintf(p, "%s/enable", path);
+
+ ret = asprintf(&p, "%s/enable", path_dirname);
+ if (ret < 0)
+ die("Failed to allocate enable path for %s", path_dirname);
ret = stat(p, &st);
if (ret >= 0)
event->enable_file = p;
@@ -2249,10 +2256,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)
- die("Failed to allocate trigger path for %s", path);
- sprintf(p, "%s/trigger", path);
+ ret = asprintf(&p, "%s/trigger", path_dirname);
+ if (ret < 0)
+ die("Failed to allocate trigger path for %s", path_dirname);
ret = stat(p, &st);
if (ret > 0)
die("trigger specified but not supported by this kernel");
@@ -2266,22 +2272,24 @@ static void make_sched_event(struct buffer_instance *instance,
struct event_list **event, struct event_list *sched,
const char *sched_path)
{
- char *path;
- char *p;
+ char *path, *sched_filter_file_tmp;
+ int ret;

/* Do nothing if the event already exists */
if (*event)
return;

- path = malloc(strlen(sched->filter_file) + strlen(sched_path) + 2);
- if (!path)
+ /* we do not want to corrupt sched->filter_file when using dirname() */
+ sched_filter_file_tmp = strdup(sched->filter_file);
+ if (!sched_filter_file_tmp)
die("Failed to allocate path for %s", sched_path);

- sprintf(path, "%s", sched->filter_file);
-
- /* Remove the /filter from filter file */
- p = path + strlen(path) - strlen("filter");
- sprintf(p, "%s/filter", sched_path);
+ ret = asprintf(&path, "%s/%s/filter",
+ dirname(sched_filter_file_tmp),
+ sched_path);
+ free(sched_filter_file_tmp);
+ if (ret < 0)
+ die("Failed to allocate path for %s", sched_path);

*event = create_event(instance, path, sched);
free(path);
@@ -2310,10 +2318,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);

@@ -3969,6 +3976,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") +
@@ -3976,10 +3985,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);
}
}

@@ -4373,7 +4381,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},
@@ -4438,12 +4446,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..fd16354 100644
--- a/trace-stat.c
+++ b/trace-stat.c
@@ -70,15 +70,16 @@ char *strstrip(char *str)
return s;
}

+/* FIXME: append_file() is duplicated and could be consolidated */
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 d464a61..45fa95a 100644
--- a/trace-util.c
+++ b/trace-util.c
@@ -85,14 +85,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);
@@ -614,14 +615,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",
@@ -707,7 +704,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;
@@ -749,16 +746,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;
}
@@ -774,16 +771,15 @@ const char *tracecmd_get_tracing_dir(void)
return tracing_dir;
}

+/* FIXME: append_file() is duplicated and could be consolidated */
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;
}

/**
@@ -1389,7 +1385,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;
@@ -1412,14 +1409,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);
@@ -1506,15 +1499,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",
@@ -1603,6 +1593,7 @@ char *tracecmd_get_tracing_file(const char *name)
{
static const char *tracing;
char *file;
+ int ret;

if (!tracing) {
tracing = tracecmd_find_tracing_dir();
@@ -1610,11 +1601,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;
}

--
2.13.3