On Tue, 18 Jun 2019 17:32:45 +0300
tz.stoyanov@xxxxxxxxx wrote:
> From: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx>
>
> [
> v2 changes:
> - Replaced usage of tracecmd_add_option_v() with tracecmd_add_option() API.
> - Added checks to prevent buffer overflow when sscanf (... "%s", buf) is used.
> - Return error in case memory allocation fails.
> - Return error if option string is not in the expected format.
> - Sort memory maps and use binary search to find matching library in the map.
> ]
Hi Tzvetomir!
Note, "v2 changes" text should be below the 3 lines after the SOB.
>
> A new trace-cmd record option is added: "--mmap". When it is set with
> combination of -F or -P options, the memory map of the traced applications
> is stored in the trace.dat file. A new API tracecmd_search_tracee_mmap()
> can be used to look up into stored memory maps. The map is retrieved from
> /proc/<pid>/maps file.
>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@xxxxxxxxx>
> ---
v2 changes:
- Replaced usage of tracecmd_add_option_v() with
tracecmd_add_option() API.
- Added checks to prevent buffer overflow when sscanf (... "%s",
buf) is used.
- Return error in case memory allocation fails.
- Return error if option string is not in the expected format.
- Sort memory maps and use binary search to find matching library
in the map.
This way a "git am" will ignore it. This is what the Linux kernel
community usually does.
> include/trace-cmd/trace-cmd.h | 4 +
> lib/trace-cmd/trace-input.c | 140 ++++++++++++++++++++++++++++++++-
> tracecmd/include/trace-local.h | 16 ++++
> tracecmd/trace-record.c | 137 ++++++++++++++++++++++++++++++++
> 4 files changed, 294 insertions(+), 3 deletions(-)
>
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index 6f62ab9..210be03 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -82,6 +82,7 @@ enum {
> TRACECMD_OPTION_OFFSET,
> TRACECMD_OPTION_CPUCOUNT,
> TRACECMD_OPTION_VERSION,
> + TRACECMD_OPTION_PIDMMAPS,
> };
>
> enum {
> @@ -208,6 +209,9 @@ unsigned long long tracecmd_page_ts(struct tracecmd_input *handle,
> unsigned int tracecmd_record_ts_delta(struct tracecmd_input *handle,
> struct tep_record *record);
>
> +char *tracecmd_get_tracee_lib(struct tracecmd_input *handle,
> + int pid, unsigned long addr);
Hmm, I rather not call it "tracee", that's a bit slang.
Perhaps just call it "task"
tracecmd_get_task_lib()
> +
> #ifndef SWIG
> /* hack for function graph work around */
> extern __thread struct tracecmd_input *tracecmd_curr_thread_handle;
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 61566ba..e2a06a8 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -101,6 +101,7 @@ struct tracecmd_input {
> struct tracecmd_ftrace finfo;
>
> struct hook_list *hooks;
> + struct pid_mem_maps *pid_mmaps;
> /* file information */
> size_t header_files_start;
> size_t ftrace_files_start;
> @@ -2134,6 +2135,138 @@ void tracecmd_set_ts2secs(struct tracecmd_input *handle,
> handle->use_trace_clock = false;
> }
>
> +static int trace_pid_mmap_cmp(const void *a, const void *b)
> +{
> + struct lib_mem_map *map_a = (struct lib_mem_map *)a;
> + struct lib_mem_map *map_b = (struct lib_mem_map *)b;
> +
> + if (map_a->start > map_b->start)
> + return 1;
> + if (map_a->start < map_b->start)
> + return -1;
> + return 0;
> +}
> +
> +#define STR_MMAP_LINE_MAX (PATH_MAX+34)
> +static int trace_pid_mmap_load(struct tracecmd_input *handle, char *buf)
> +{
> + struct pid_mem_maps *maps = NULL;
> + char mapname[STR_MMAP_LINE_MAX];
> + char *line;
> + int ret;
> + int i;
> +
> + maps = calloc(1, sizeof(*maps));
> + if (!maps)
> + return -ENOMEM;
> +
> + line = strchr(buf, '\n');
> + if (!line) {
> + free(maps);
> + return -EINVAL;
> + }
> + *line = '\0';
> + if (strlen(buf) > STR_MMAP_LINE_MAX) {
> + free(maps);
> + return -EINVAL;
> + }
> + ret = sscanf(buf, "%x %x %s", &maps->pid, &maps->nr_lib_maps, mapname);
> + if (ret != 3) {
> + free(maps);
> + return -EINVAL;
> + }
> + maps->proc_name = strdup(mapname);
> + if (!maps->proc_name) {
> + free(maps);
> + return -ENOMEM;
> + }
> + maps->lib_maps = calloc(maps->nr_lib_maps, sizeof(struct lib_mem_map));
> + if (!maps->lib_maps) {
> + free(maps->proc_name);
> + free(maps);
> + return -ENOMEM;
> + }
BTW, when you have a lot failure paths like this, you can make do the
following:
ret = -EINVAL;
line = strchr(buf, '\n');
if (!line)
goto out_fail;
[..]
if (strlen(buf) > STR_MMAP_LINE_MAX)
goto out_fail;
[..]
if (ret != 3)
goto out_fail;
ret = -ENOMEM;
maps->proc_name = strdup(mapname);
if (!maps->proc_name)
goto out_fail;
[..]
if (!maps->lib_maps)
goto out_fail;
> + maps->next = handle->pid_mmaps;
> + handle->pid_mmaps = maps;
> + buf = line+1;
> + line = strchr(buf, '\n');
> + for (i = 0; i < maps->nr_lib_maps; i++) {
> + if (!line)
> + break;
> + *line = '\0';
> + if (strlen(buf) > STR_MMAP_LINE_MAX)
> + break;
> + ret = sscanf(buf, "%llx %llx %s", &maps->lib_maps[i].start,
> + &maps->lib_maps[i].end, mapname);
> + if (ret == 3) {
> + maps->lib_maps[i].lib_name = strdup(mapname);
> + if (!maps->lib_maps[i].lib_name)
> + return -ENOMEM;
Hmm, perhaps we should wait to update handle->pid_maps until after
this, and have this goto out_fail as well.
> + }
> + buf = line+1;
> + line = strchr(buf, '\n');
> + }
> +
> + if (i != maps->nr_lib_maps)
Here too.
> + return -EINVAL;
> +
> + qsort(maps->lib_maps, maps->nr_lib_maps,
> + sizeof(*maps->lib_maps), trace_pid_mmap_cmp);
Here we should update handle->pid_mmaps = maps;
> +
> + return 0;
Here we have:
out_fail:
free(maps->lib_maps);
free(maps->proc_name);
free(maps);
return ret;
> +}
> +
> +static int trace_pid_mmap_search(const void *a, const void *b)
> +{
> + struct lib_mem_map *key = (struct lib_mem_map *)a;
> + struct lib_mem_map *map = (struct lib_mem_map *)b;
> +
> + if (key->start > map->end)
> + return 1;
> + if (key->start < map->start)
> + return -1;
> + return 0;
> +}
> +
> +/**
> + * tracecmd_search_tracee_mmap - Search tracee memory address map
Probably want to call this tracecmd_search_task_mmap().
> + * @handle: input handle to the trace.dat file
> + * @pid: pid of the tracee
of the task
> + * @addr: address from the tracee memory space.
> + *
> + * Map of the tracee memory can be saved in the trace.dat file, using the option
> + * "--mmap". If there is such information, this API can be used to look up into
> + * this memory map to find library is loaded at the given @addr.
> + *
> + * The name of the library at given tracee @addr is returned.
> + */
> +char *tracecmd_search_tracee_mmap(struct tracecmd_input *handle,
> + int pid, unsigned long long addr)
> +{
> + struct pid_mem_maps *maps;
> + struct lib_mem_map *lib;
> + struct lib_mem_map key;
> +
> + if (!handle || !handle->pid_mmaps)
> + return NULL;
> +
> + maps = handle->pid_mmaps;
> + while (maps) {
> + if (maps->pid == pid)
> + break;
> + maps = maps->next;
> + }
> + if (!maps || !maps->nr_lib_maps || !maps->lib_maps)
> + return NULL;
> + key.start = addr;
> + lib = bsearch(&key, maps->lib_maps, maps->nr_lib_maps,
> + sizeof(*maps->lib_maps), trace_pid_mmap_search);
> + if (lib)
> + return lib->lib_name;
> +
> + return NULL;
> +}
> +
> static int handle_options(struct tracecmd_input *handle)
> {
> unsigned long long offset;
> @@ -2221,9 +2354,6 @@ static int handle_options(struct tracecmd_input *handle)
> case TRACECMD_OPTION_UNAME:
> handle->uname = strdup(buf);
> break;
> - case TRACECMD_OPTION_VERSION:
> - handle->version = strdup(buf);
> - break;
> case TRACECMD_OPTION_HOOK:
> hook = tracecmd_create_event_hook(buf);
> hook->next = handle->hooks;
> @@ -2233,6 +2363,10 @@ static int handle_options(struct tracecmd_input *handle)
> cpus = *(int *)buf;
> handle->cpus = tep_read_number(handle->pevent, &cpus, 4);
> break;
> + case TRACECMD_OPTION_PIDMMAPS:
> + if (buf[size] == '\0')
> + trace_pid_mmap_load(handle, buf);
> + break;
> default:
> warning("unknown option %d", option);
> break;
> diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h
> index 1cad3cc..dc4dc03 100644
> --- a/tracecmd/include/trace-local.h
> +++ b/tracecmd/include/trace-local.h
> @@ -157,6 +157,20 @@ struct func_list {
> const char *mod;
> };
>
> +struct lib_mem_map {
> + unsigned long long start;
> + unsigned long long end;
> + char *lib_name;
> +};
> +
> +struct pid_mem_maps {
> + struct pid_mem_maps *next;
> + struct lib_mem_map *lib_maps;
> + unsigned int nr_lib_maps;
> + char *proc_name;
> + int pid;
> +};
> +
> struct buffer_instance {
> struct buffer_instance *next;
> const char *name;
> @@ -183,6 +197,8 @@ struct buffer_instance {
> struct tracecmd_msg_handle *msg_handle;
> struct tracecmd_output *network_handle;
>
> + struct pid_mem_maps *mem_maps;
> +
> char *max_graph_depth;
>
> int flags;
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 5dc6f17..a938ce3 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -84,6 +84,7 @@ static int max_kb;
> static bool use_tcp;
>
> static int do_ptrace;
> +static int get_mmap;
>
> static int filter_task;
> static int filter_pid = -1;
> @@ -1062,6 +1063,98 @@ static char *make_pid_filter(char *curr_filter, const char *field)
> return filter;
> }
>
> +static int get_pid_mmaps(int pid)
> +{
> + struct buffer_instance *instance = &top_instance;
> + struct pid_mem_maps *maps = instance->mem_maps;
> + unsigned long long begin, end, inode, tmp;
> + struct lib_mem_map *map;
> + char mapname[PATH_MAX+1];
> + char fname[PATH_MAX+1];
> + char buf[PATH_MAX+100];
> + char perm[5];
> + char dev[6];
> + FILE *f;
> + int ret;
> + int res;
> + int i;
> +
> + while (maps) {
> + if (pid == maps->pid)
> + break;
> + maps = maps->next;
> + }
BTW, each time I look at this code, I think this is a bug because you
don't check for (!maps) at the end of this loop. But then notice below
you do. Let's move this while loop to just before the:
if (!maps) {
As it makes more sense to keep them together, and less likely that we
think maps is used as NULL.
> +
> + sprintf(fname, "/proc/%d/exe", pid);
> + ret = readlink(fname, mapname, PATH_MAX);
> + if (ret >= PATH_MAX || ret < 0)
> + return -ENOENT;
> +
> + sprintf(fname, "/proc/%d/maps", pid);
> + f = fopen(fname, "r");
> + if (!f)
> + return -ENOENT;
> +
> + if (!maps) {
> + maps = calloc(1, sizeof(*maps));
> + if (!maps)
> + return -ENOMEM;
> + maps->pid = pid;
> + maps->next = instance->mem_maps;
> + instance->mem_maps = maps;
> + } else {
> + for (i = 0; i < maps->nr_lib_maps; i++)
> + free(maps->lib_maps[i].lib_name);
> + free(maps->lib_maps);
> + maps->lib_maps = NULL;
> + maps->nr_lib_maps = 0;
> + free(maps->proc_name);
> + }
> +
> + maps->proc_name = strdup(mapname);
> + if (!maps->proc_name)
> + return -ENOMEM;
We need to close 'f' before we return an error. Let's do:
ret = -ENOMEM;
maps->proc_name = strdup(mapname);
if (!maps->proc_name)
goto out;
> +
> + ret = 0;
> + while (fgets(buf, sizeof(buf), f)) {
> + mapname[0] = '\0';
> + res = sscanf(buf, "%llx-%llx %4s %llx %5s %lld %s",
> + &begin, &end, perm, &tmp, dev, &inode, mapname);
> + if (res == 7 && mapname[0] != '\0') {
> + map = realloc(maps->lib_maps,
> + (maps->nr_lib_maps+1)*sizeof(*map));
> + if (!map) {
> + ret = -ENOMEM;
> + break;
> + }
> + map[maps->nr_lib_maps].end = end;
> + map[maps->nr_lib_maps].start = begin;
> + map[maps->nr_lib_maps].lib_name = strdup(mapname);
> + if (!map[maps->nr_lib_maps].lib_name) {
> + ret = -ENOMEM;
> + free(map);
> + break;
I'm thinking on error, we goto "out_fail" and remove all maps.
> + }
> + maps->lib_maps = map;
> + maps->nr_lib_maps++;
> + }
> + }
out:
> + fclose(f);
> +
> + return ret;
out_fail:
for (i = 0; i < maps->nr_lib_maps; i++) {
free(maps->lib_maps[i].lib_name);
free(maps->lib_maps);
But we would also need to remove it from the instance->mem_maps list.
-- Steve
> +}
> +
> +static void get_filter_pid_mmaps(void)
> +{
> + struct filter_pids *p;
> +
> + for (p = filter_pids; p; p = p->next) {
> + if (p->exclude)
> + continue;
> + get_pid_mmaps(p->pid);
> + }
> +}
> +
> static void update_task_filter(void)
> {
> struct buffer_instance *instance;
> @@ -1070,6 +1163,9 @@ static void update_task_filter(void)
> if (no_filter)
> return;
>
> + if (get_mmap && filter_pids)
> + get_filter_pid_mmaps();
> +
> if (filter_task)
> add_filter_pid(pid, 0);
>
> @@ -1264,6 +1360,8 @@ static void ptrace_wait(enum trace_type type, int main_pid)
> break;
>
> case PTRACE_EVENT_EXIT:
> + if (get_mmap)
> + get_pid_mmaps(main_pid);
> ptrace(PTRACE_GETEVENTMSG, pid, NULL, &cstatus);
> ptrace(PTRACE_DETACH, pid, NULL, NULL);
> break;
> @@ -3094,6 +3192,33 @@ static void append_buffer(struct tracecmd_output *handle,
> }
> }
>
> +
> +static void
> +add_pid_mem_maps(struct tracecmd_output *handle, struct buffer_instance *instance)
> +{
> + struct pid_mem_maps *maps = instance->mem_maps;
> + struct trace_seq s;
> + int i;
> +
> + trace_seq_init(&s);
> + while (maps) {
> + if (!maps->nr_lib_maps)
> + continue;
> + trace_seq_reset(&s);
> + trace_seq_printf(&s, "%x %x %s\n",
> + maps->pid, maps->nr_lib_maps, maps->proc_name);
> + for (i = 0; i < maps->nr_lib_maps; i++)
> + trace_seq_printf(&s, "%llx %llx %s\n",
> + maps->lib_maps[i].start,
> + maps->lib_maps[i].end,
> + maps->lib_maps[i].lib_name);
> + tracecmd_add_option(handle, TRACECMD_OPTION_PIDMMAPS,
> + s.len+1, s.buffer);
> + maps = maps->next;
> + }
> + trace_seq_destroy(&s);
> +}
> +
> static void
> add_buffer_stat(struct tracecmd_output *handle, struct buffer_instance *instance)
> {
> @@ -3287,6 +3412,10 @@ static void record_data(struct common_record_context *ctx)
> if (!no_top_instance() && !top_instance.msg_handle)
> print_stat(&top_instance);
>
> + for_all_instances(instance) {
> + add_pid_mem_maps(handle, instance);
> + }
> +
> tracecmd_append_cpu_data(handle, local_cpu_count, temp_files);
>
> for (i = 0; i < max_cpu_count; i++)
> @@ -4397,6 +4526,7 @@ void update_first_instance(struct buffer_instance *instance, int topt)
> }
>
> enum {
> + OPT_mmap = 244,
> OPT_quiet = 245,
> OPT_debug = 246,
> OPT_no_filter = 247,
> @@ -4627,6 +4757,7 @@ static void parse_record_options(int argc,
> {"debug", no_argument, NULL, OPT_debug},
> {"quiet", no_argument, NULL, OPT_quiet},
> {"help", no_argument, NULL, '?'},
> + {"mmap", no_argument, NULL, OPT_mmap},
> {"module", required_argument, NULL, OPT_module},
> {NULL, 0, NULL, 0}
> };
> @@ -4858,6 +4989,9 @@ static void parse_record_options(int argc,
> case 'i':
> ignore_event_not_found = 1;
> break;
> + case OPT_mmap:
> + get_mmap = 1;
> + break;
> case OPT_date:
> ctx->date = 1;
> if (ctx->data_flags & DATA_FL_OFFSET)
> @@ -4924,6 +5058,9 @@ static void parse_record_options(int argc,
> add_func(&ctx->instance->filter_funcs,
> ctx->instance->filter_mod, "*");
>
> + if (filter_task && get_mmap)
> + do_ptrace = 1;
> +
> if (do_ptrace && !filter_task && (filter_pid < 0))
> die(" -c can only be used with -F (or -P with event-fork support)");
> if (ctx->do_child && !filter_task &&! filter_pid)
|