Re: [PATCH v3] trace-cmd: Save the tracee memory map into the trace.dat file.

["Yordan Karadzhov (VMware)" <y.karadz@gmail.com>]

On 19.06.19 г. 11:39 ч., tz.stoyanov@gmail.com wrote:
> From: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
> 
> 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_task_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@gmail.com>
> ---
> [
>   v3 changes:
>     - Changed tracecmd_search_task_mmap() API to return not only the library
>       name, but also the start and end memory addresses.
>     - Renamed *tracee* to *task*
>     - Improved resources cleanup, in case of an error.
>     - Removed (this) changelog from the commit message.
> 
>   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.
> ]
> 
>   include/trace-cmd/trace-cmd.h  |   9 ++
>   lib/trace-cmd/trace-input.c    | 149 ++++++++++++++++++++++++++++++-
>   tracecmd/include/trace-local.h |  10 +++
>   tracecmd/trace-record.c        | 158 +++++++++++++++++++++++++++++++++
>   4 files changed, 323 insertions(+), 3 deletions(-)
> 
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index 6f62ab9..17edb9d 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,
>   };

...

>   
> +static int get_pid_mmaps(int pid)
> +{
> +	struct buffer_instance *instance = &top_instance;
> +	struct pid_mem_maps *maps = instance->mem_maps;
> +	struct pid_mem_maps *m;
> +	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;
> +
> +	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;
> +
> +	while (maps) {
> +		if (pid == maps->pid)
> +			break;
> +		maps = maps->next;
> +	}
> +
> +	ret = -ENOMEM;
> +	if (!maps) {
> +		maps = calloc(1, sizeof(*maps));
> +		if (!maps)
> +			goto out_fail;
> +		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)
> +		goto out;
> +
> +	while (fgets(buf, sizeof(buf), f)) {
> +		mapname[0] = '\0';
> +		res = sscanf(buf, "%llx-%llx %4s %llx %5s %lld %s",

We tried testing the patch and we found that the memory map file is not 
properly parsed. The problem seems to get solved if we replace the line 
above with
		res = sscanf(buf, "%llx-%llx %4s %llx %s %lld %s",

Thanks!
Yordan

> +			     &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)
> +				goto out_fail;
> +			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)
> +				goto out_fail;
> +			maps->lib_maps = map;
> +			maps->nr_lib_maps++;
> +		}
> +	}
> +out:
> +	fclose(f);
> +	return 0;
> +
> +out_fail:
> +	fclose(f);
> +	if (maps) {
> +		for (i = 0; i < maps->nr_lib_maps; i++)
> +			free(maps->lib_maps[i].lib_name);
> +		if (instance->mem_maps != maps) {
> +			m = instance->mem_maps;
> +			while (m) {
> +				if (m->next == maps) {
> +					m->next = maps->next;
> +					break;
> +				}
> +				m = m->next;
> +			}
> +		} else
> +			instance->mem_maps = maps->next;
> +		free(maps->lib_maps);
> +		maps->lib_maps = NULL;
> +		maps->nr_lib_maps = 0;
> +		free(maps->proc_name);
> +		maps->proc_name = NULL;
> +		free(maps);
> +	}
> +	return ret;
> +}
> +
> +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 +1184,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 +1381,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 +3213,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 +3433,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 +4547,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 +4778,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 +5010,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 +5079,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)
>