Re: [PATCH] perf: fix uninitialized variable in thread__set_comm()

[Stephane Eranian]

On Fri, Oct 7, 2011 at 10:45 PM, Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> wrote:
> Em Fri, Oct 07, 2011 at 12:14:18PM +0200, Stephane Eranian escreveu:
>> Arnaldo,
>>
>> Did you also integrate this patch?
>>
>>
>> On Tue, Sep 6, 2011 at 5:00 PM, Stephane Eranian <eranian@xxxxxxxxxx> wrote:
>> >
>> > In thread__set_comm(), a command string could be freed and then
>> > reused to point to a new command. Yet, the length was never reset
>> > nor recomputed. This could lead to bogus column formatting.
>
> Perhaps I'm out of coffee right now, but how can it be reused in such a
> way?
>
you come into thread__set_comm() with self->com != NULL,
you free() self->comm, you strdup() the new command, as far as I know it
may have a different length from what you just freed, err = 0, set comm_set
to skip strlen() later on. And you have a stale self->comm_len.
At least that's my understanding of the code. I got there because I noticed
there was something wrong with column formatting.


> I don't have a problem in doing the strlen in thread__set_comm, just
> want to understand the sequence of events that lead to this
> inconsistency.
>
> - Arnaldo
>
>> > This patch below fixes the problem by having thread__set_comm()
>> > immediately compute the string length in thread->comm_len. It
>> > gets rid of thread__comm_len() which is not needed anymore.
>> > It was doing the lazy eval of strlen() which could cause the
>> > issue.
>> >
>> > Signed-off-by: Stephane Eranian <eranian@xxxxxxxxxx>
>> > ---
>> >
>> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
>> > index 677e1da..b883821 100644
>> > --- a/tools/perf/util/hist.c
>> > +++ b/tools/perf/util/hist.c
>> > @@ -61,7 +61,7 @@ static void hists__calc_col_len(struct hists *self, struct hist_entry *h)
>> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â unresolved_col_width);
>> > Â Â Â Â}
>> >
>> > - Â Â Â len = thread__comm_len(h->thread);
>> > + Â Â Â len = h->thread->comm_len;
>> > Â Â Â Âif (hists__new_col_len(self, HISTC_COMM, len))
>> > Â Â Â Â Â Â Â Âhists__set_col_len(self, HISTC_THREAD, len + 6);
>> >
>> > @@ -731,7 +731,7 @@ static size_t hist_entry__fprintf_callchain(struct hist_entry *self,
>> > Â Â Â Â Â Â Â Âstruct sort_entry *se = list_first_entry(&hist_entry__sort_list,
>> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â typeof(*se), list);
>> > Â Â Â Â Â Â Â Âleft_margin = hists__col_len(hists, se->se_width_idx);
>> > - Â Â Â Â Â Â Â left_margin -= thread__comm_len(self->thread);
>> > + Â Â Â Â Â Â Â left_margin -= self->thread->comm_len;
>> > Â Â Â Â}
>> >
>> > Â Â Â Âreturn hist_entry_callchain__fprintf(fp, self, session_total,
>> > diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
>> > index d5d3b22..3ce2a80 100644
>> > --- a/tools/perf/util/thread.c
>> > +++ b/tools/perf/util/thread.c
>> > @@ -35,26 +35,18 @@ int thread__set_comm(struct thread *self, const char *comm)
>> >
>> > Â Â Â Âif (self->comm)
>> > Â Â Â Â Â Â Â Âfree(self->comm);
>> > +
>> > Â Â Â Âself->comm = strdup(comm);
>> > +
>> > Â Â Â Âerr = self->comm == NULL ? -ENOMEM : 0;
>> > Â Â Â Âif (!err) {
>> > Â Â Â Â Â Â Â Âself->comm_set = true;
>> > + Â Â Â Â Â Â Â self->comm_len = strlen(self->comm);
>> > Â Â Â Â Â Â Â Âmap_groups__flush(&self->mg);
>> > Â Â Â Â}
>> > Â Â Â Âreturn err;
>> > Â}
>> >
>> > -int thread__comm_len(struct thread *self)
>> > -{
>> > - Â Â Â if (!self->comm_len) {
>> > - Â Â Â Â Â Â Â if (!self->comm)
>> > - Â Â Â Â Â Â Â Â Â Â Â return 0;
>> > - Â Â Â Â Â Â Â self->comm_len = strlen(self->comm);
>> > - Â Â Â }
>> > -
>> > - Â Â Â return self->comm_len;
>> > -}
>> > -
>> > Âstatic size_t thread__fprintf(struct thread *self, FILE *fp)
>> > Â{
>> > Â Â Â Âreturn fprintf(fp, "Thread %d %s\n", self->pid, self->comm) +
>> > diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
>> > index e5f2401..701ea09 100644
>> > --- a/tools/perf/util/thread.h
>> > +++ b/tools/perf/util/thread.h
>> > @@ -23,7 +23,6 @@ struct perf_session;
>> > Âvoid thread__delete(struct thread *self);
>> >
>> > Âint thread__set_comm(struct thread *self, const char *comm);
>> > -int thread__comm_len(struct thread *self);
>> > Âstruct thread *perf_session__findnew(struct perf_session *self, pid_t pid);
>> > Âvoid thread__insert_map(struct thread *self, struct map *map);
>> > Âint thread__fork(struct thread *self, struct thread *parent);
>> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/