* [PATCH] perf: fix uninitialized variable in thread__set_comm()
@ 2011-09-06 15:00 Stephane Eranian
2011-10-07 10:14 ` Stephane Eranian
0 siblings, 1 reply; 4+ messages in thread
From: Stephane Eranian @ 2011-09-06 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: acme, peterz, mingo, dsahern
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.
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@google.com>
---
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);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] perf: fix uninitialized variable in thread__set_comm()
2011-09-06 15:00 [PATCH] perf: fix uninitialized variable in thread__set_comm() Stephane Eranian
@ 2011-10-07 10:14 ` Stephane Eranian
2011-10-07 20:45 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 4+ messages in thread
From: Stephane Eranian @ 2011-10-07 10:14 UTC (permalink / raw)
To: linux-kernel; +Cc: acme, peterz, mingo, dsahern
Arnaldo,
Did you also integrate this patch?
On Tue, Sep 6, 2011 at 5:00 PM, Stephane Eranian <eranian@google.com> 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.
>
> 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@google.com>
> ---
>
> 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);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] perf: fix uninitialized variable in thread__set_comm()
2011-10-07 10:14 ` Stephane Eranian
@ 2011-10-07 20:45 ` Arnaldo Carvalho de Melo
2011-10-07 21:53 ` Stephane Eranian
0 siblings, 1 reply; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2011-10-07 20:45 UTC (permalink / raw)
To: Stephane Eranian; +Cc: linux-kernel, peterz, mingo, dsahern
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@google.com> 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?
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@google.com>
> > ---
> >
> > 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);
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] perf: fix uninitialized variable in thread__set_comm()
2011-10-07 20:45 ` Arnaldo Carvalho de Melo
@ 2011-10-07 21:53 ` Stephane Eranian
0 siblings, 0 replies; 4+ messages in thread
From: Stephane Eranian @ 2011-10-07 21:53 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, peterz, mingo, dsahern
On Fri, Oct 7, 2011 at 10:45 PM, Arnaldo Carvalho de Melo
<acme@redhat.com> 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@google.com> 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@google.com>
>> > ---
>> >
>> > 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);
>> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-10-07 21:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-06 15:00 [PATCH] perf: fix uninitialized variable in thread__set_comm() Stephane Eranian
2011-10-07 10:14 ` Stephane Eranian
2011-10-07 20:45 ` Arnaldo Carvalho de Melo
2011-10-07 21:53 ` Stephane Eranian
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox