perf: fix uninitialized variable in thread__set_comm()

PATCH In review Stephane Eranian 2 participants 4 messages Sep 6, 2011 – Oct 7, 2011 perf events

Share & tools

Stephane Eranian

author

2011-09-06 15:00 UTC #0 lore raw

PATCH 1/1 perf: fix uninitialized variable in thread__set_comm()

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 <[email protected]>

---

tools/perf/util/hist.c +2 -2

index 677e1da..b883821 100644
@@ -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,

tools/perf/util/thread.c +3 ~~-11~~

index d5d3b22..3ce2a80 100644
@@ -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) +

tools/perf/util/thread.h +0 -1

index e5f2401..701ea09 100644
@@ -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);

Stephane Eranian

author

2011-10-07 10:14 UTC #1 lore raw

Arnaldo,

Did you also integrate this patch?

Quoted 83 lines · Stephane Eranian

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.

Signed-off-by Stephane Eranian <[email protected]>

---

tools/perf/util/hist.c +2 -2

index 677e1da..b883821 100644
@@ -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,

tools/perf/util/thread.c +3 ~~-11~~

index d5d3b22..3ce2a80 100644
@@ -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) +

tools/perf/util/thread.h +0 -1

index e5f2401..701ea09 100644
@@ -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);

Arnaldo Carvalho de Melo

2011-10-07 20:45 UTC #2 lore raw

Em Fri, Oct 07, 2011 at 12:14:18PM +0200, Stephane Eranian escreveu:

Quoted 10 lines

Arnaldo,
Did you also integrate this patch?
Quoted 4 lines · Stephane Eranian
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

Quoted 78 lines

Signed-off-by Stephane Eranian <[email protected]>

---

tools/perf/util/hist.c +2 -2

index 677e1da..b883821 100644
@@ -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,

tools/perf/util/thread.c +3 ~~-11~~

index d5d3b22..3ce2a80 100644
@@ -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) +

tools/perf/util/thread.h +0 -1

index e5f2401..701ea09 100644
@@ -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);

Stephane Eranian

author

2011-10-07 21:53 UTC #3 lore raw

Quoted 6 lines · Arnaldo Carvalho de Melo

Em Fri, Oct 07, 2011 at 12:14:18PM +0200, Stephane Eranian escreveu:
Quoted 5 lines
Arnaldo,
Did you also integrate this patch?

Quoted 9 lines

Quoted 5 lines · Stephane Eranian
Quoted 4 lines
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.

Quoted 85 lines

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

Quoted 78 lines

Signed-off-by Stephane Eranian <[email protected]>

---

tools/perf/util/hist.c +2 -2

index 677e1da..b883821 100644
@@ -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,

tools/perf/util/thread.c +3 ~~-11~~

index d5d3b22..3ce2a80 100644
@@ -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) +

tools/perf/util/thread.h +0 -1

index e5f2401..701ea09 100644
@@ -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);