Re: [PATCH 2/5] perf tools: fix piped mode read code

[David Ahern <dsahern@gmail.com>]

Hi Stephane:

This patch no longer applies cleanly. Can you update the series?

David

On 4/11/12 3:01 AM, Stephane Eranian wrote:
> In __perf_session__process_pipe_events(), there was a risk
> we would read more than what a union perf_event struct can
> hold. this could happen in case, perf is reading a file which
> contains new record types it does not know about and which are
> larger than anything it knows about.
>
> In general, perf is supposed to skip records it does not
> understand, but in pipe mode, those have to be read and ignored.
> The fixed size header contains the size of the record, but that
> size may be larger than union perf_event, yet it was used as
> the backing to the read in:
>
>    union perf_event event;
>    void *p;
>
>    size = event->header.size;
>
>    p =&event;
>    p += sizeof(struct perf_event_header);
>    if (size - sizeof(struct perf_event_header)) {
>      err = readn(self->fd, p, size - sizeof(struct perf_event_header));
>
> We fix this by allocating a buffer based on the size reported in
> the header. We reuse the buffer as much as we can. We realloc in
> case it becomes too small. In the  common case, the performance
> impact is negligible.
>
> Signed-off-by: Stephane Eranian<eranian@google.com>
> ---
>   tools/perf/util/session.c |   35 +++++++++++++++++++++++++++--------
>   1 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 9412e3b..d13e915 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1056,8 +1056,9 @@ volatile int session_done;
>   static int __perf_session__process_pipe_events(struct perf_session *self,
>   					       struct perf_tool *tool)
>   {
> -	union perf_event event;
> -	uint32_t size;
> +	union perf_event *event;
> +	uint32_t size, cur_size = 0;
> +	void *buf = NULL;
>   	int skip = 0;
>   	u64 head;
>   	int err;
> @@ -1066,8 +1067,14 @@ static int __perf_session__process_pipe_events(struct perf_session *self,
>   	perf_tool__fill_defaults(tool);
>
>   	head = 0;
> +	cur_size = sizeof(union perf_event);
> +
> +	buf = malloc(cur_size);
> +	if (!buf)
> +		return -errno;
>   more:
> -	err = readn(self->fd,&event, sizeof(struct perf_event_header));
> +	event = buf;
> +	err = readn(self->fd, event, sizeof(struct perf_event_header));
>   	if (err<= 0) {
>   		if (err == 0)
>   			goto done;
> @@ -1077,13 +1084,23 @@ static int __perf_session__process_pipe_events(struct perf_session *self,
>   	}
>
>   	if (self->header.needs_swap)
> -		perf_event_header__bswap(&event.header);
> +		perf_event_header__bswap(&event->header);
>
> -	size = event.header.size;
> +	size = event->header.size;
>   	if (size == 0)
>   		size = 8;
>
> -	p =&event;
> +	if (size>  cur_size) {
> +		void *new = realloc(buf, size);
> +		if (!new) {
> +			pr_err("failed to allocate memory to read event\n");
> +			goto out_err;
> +		}
> +		buf = new;
> +		cur_size = size;
> +		event = buf;
> +	}
> +	p = event;
>   	p += sizeof(struct perf_event_header);
>
>   	if (size - sizeof(struct perf_event_header)) {
> @@ -1099,9 +1116,10 @@ static int __perf_session__process_pipe_events(struct perf_session *self,
>   		}
>   	}
>
> -	if ((skip = perf_session__process_event(self,&event, tool, head))<  0) {
> +	skip = perf_session__process_event(self, event, tool, head);
> +	if (skip<  0) {
>   		dump_printf("%#" PRIx64 " [%#x]: skipping unknown header type: %d\n",
> -			    head, event.header.size, event.header.type);
> +			    head, event->header.size, event->header.type);
>   		/*
>   		 * assume we lost track of the stream, check alignment, and
>   		 * increment a single u64 in the hope to catch on again 'soon'.
> @@ -1122,6 +1140,7 @@ static int __perf_session__process_pipe_events(struct perf_session *self,
>   done:
>   	err = 0;
>   out_err:
> +	free(buf);
>   	perf_session__warn_about_errors(self, tool);
>   	perf_session_free_sample_buffers(self);
>   	return err;