Messages in this thread | | | From | Stephane Eranian <> | Date | Fri, 10 Aug 2018 01:21:18 -0700 | Subject | Re: [PATCH v2] perf ordered_events: fix crash in free_dup_event() |
| |
On Thu, Aug 9, 2018 at 1:07 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Wed, Aug 08, 2018 at 03:33:20PM -0700, Stephane Eranian wrote: > > This patch fixes a bug in ordered_event.c:alloc_event(). > > An ordered_event struct was not initialized properly potentially > > causing crashes later on in free_dup_event() depending on the > > content of the memory. If it was NULL, then it would work fine, > > otherwise, it could cause crashes such as: > > I'm now little puzzled what do we use this first event for.. > I can't see anything special about it, other than it's added > on the list uninitialized ;-) > > it seems to work properly when we ditch it.. might be some > prehistoric leftover or I'm terribly missing something > You need to keep track of the buffers to free. You do not free the ordered_event structs individually. For each oe->buffer, you need one free(). Each buffer is put in the to_free list. But to link it into the list it needs a list_head. This is what buffer[0] is used for. But the logic is broken in ordered_events__free(). It does not free individual ordered_event structs, but a buffer with many. Yet, it is missing freeing all of the duped events.
void ordered_events__free(struct ordered_events *oe) { while (!list_empty(&oe->to_free)) { struct ordered_event *buffer;
buffer = list_entry(oe->to_free.next, struct ordered_event, list); list_del(&buffer->list); ----> free_dup_event(oe, event->event); free(buffer); } } This only frees the dup_event of buffer[0] which we know is NULL (well, now). It needs to walk all the entries in buffer[] to free buffer[x].event.
I think the goal was likely to avoid adding another list_head field to each ordered_event and instead use one per allocated buffer. This is very convoluted and prone to errors and we are seeing right now. This should be cleaned. So either you add a list_head to ordered_event or you would buffer[x] in ordered_events_free().
At this point, this is my understanding. Do you agree?
> > thanks, > jirka > > > --- > diff --cc tools/perf/util/ordered-events.c > index bad9e0296e9a,0e837b0b8582..000000000000 > --- a/tools/perf/util/ordered-events.c > +++ b/tools/perf/util/ordered-events.c > @@@ -119,12 -119,8 +119,9 @@@ static struct ordered_event *alloc_even > pr("alloc size %" PRIu64 "B (+%zu), max %" PRIu64 "B\n", > oe->cur_alloc_size, size, oe->max_alloc_size); > > + oe->cur_alloc_size += size; > - list_add(&oe->buffer->list, &oe->to_free); > - > - /* First entry is abused to maintain the to_free list. */ > - oe->buffer_idx = 2; > - new = oe->buffer + 1; > + oe->buffer_idx = 1; > + new = oe->buffer; > } else { > pr("allocation limit reached %" PRIu64 "B\n", oe->max_alloc_size); > }
| |