linux-kernel - Re: [PATCH] perf: fix uninitialized variable in thread__set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CABPqkBRNJ4C5tbVnZdJB7UtySrQse2WsdC=t6kP-TQH1=7EU4w@mail.gmail.com>
Date:	Fri, 7 Oct 2011 23:53:47 +0200
From:	Stephane Eranian <eranian@...gle.com>
To:	Arnaldo Carvalho de Melo <acme@...hat.com>
Cc:	linux-kernel@...r.kernel.org, peterz@...radead.org, mingo@...e.hu,
	dsahern@...il.com
Subject: Re: [PATCH] perf: fix uninitialized variable in thread__set_comm()

On Fri, Oct 7, 2011 at 10:45 PM, Arnaldo Carvalho de Melo
<acme@...hat.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@...gle.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@...gle.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);
>> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/