Re: [PATCH] perf: fix pipe mode read code

[David Ahern <dsahern@gmail.com>]

On 2/24/12 3:12 AM, Stephane Eranian wrote:
> Any comment on this patch?
>
> On Thu, Jan 19, 2012 at 6:49 PM, Stephane Eranian<eranian@google.com>  wrote:
>>
>> In __perf_session__process_pipe_events(), there is a risk
>> we could read more than what a union perf_event struct can
>> hold. This could happen when perf is reading a file which
>> contains new and unknown record types which are larger than
>> anything the tool already knows about (i.e. part of union
>> perf_event).
>>
>> In general, perf is supposed to skip records it does not
>> understand, but in pipe mode, those have to be read and
>> ignored. They cannot just be skipped. In the current code,
>> the backing for the read is provided by union perf_event.
>> There is no check for the size limit thus there is a risk
>> of buffer overrun:
>>
>>       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));
>>
>> It should be noted that the same problem may occur with known
>> record types if they have a variable size body (not captured in
>> union perf_event).
>>
>> 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>
>> ---
>>
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index b5ca255..7f078a6 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -972,8 +972,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;
>> @@ -982,8 +983,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;
>> @@ -993,13 +1000,22 @@ 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) {
>> +               buf = realloc(buf, size);

Arnaldo pointed out recently this leaks memory if realloc failed. Need 
to save buf before the call ...

>> +               if (!buf) {

... and free on this leg.

David

>> +                       pr_err("failed to allocate memory to read event\n");
>> +                       goto out_err;
>> +               }
>> +               cur_size = size;
>> +               event = buf;
>> +       }
>> +       p = event;
>>         p += sizeof(struct perf_event_header);
>>
>>         if (size - sizeof(struct perf_event_header)) {
>> @@ -1015,9 +1031,9 @@ static int __perf_session__process_pipe_events(struct perf_session *self,
>>                 }
>>         }
>>
>> -       if ((skip = perf_session__process_event(self,&event, tool, head))<  0) {
>> +       if ((skip = perf_session__process_event(self, event, tool, head))<  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'.
>> @@ -1038,6 +1054,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;