2022-01-24 19:24:16

by Ameer Hamza

Subject: [PATCH] perf session: check for null pointer before derefernce

Move null pointer check before dereferncing the variable

Addresses-Coverity: 1497622 ("Derereference before null check")

Signed-off-by: Ameer Hamza <[email protected]>
---
tools/perf/util/session.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index f19348dddd55..e1014ab62c10 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1503,11 +1503,11 @@ static int machines__deliver_event(struct machines *machines,
++evlist->stats.nr_unknown_id;
return 0;
}
- dump_sample(evsel, event, sample, perf_env__arch(machine->env));
if (machine == NULL) {
++evlist->stats.nr_unprocessable_samples;
return 0;
}
+ dump_sample(evsel, event, sample, perf_env__arch(machine->env));
return evlist__deliver_sample(evlist, tool, event, sample, evsel, machine);
case PERF_RECORD_MMAP:
return tool->mmap(tool, event, sample, machine);
--
2.25.1

2022-01-24 19:27:37

by James Clark

Subject: Re: [PATCH] perf session: check for null pointer before derefernce

On 24/01/2022 15:00, Ameer Hamza wrote:
> Move null pointer check before dereferncing the variable
>
> Addresses-Coverity: 1497622 ("Derereference before null check")
>
> Signed-off-by: Ameer Hamza <[email protected]>
> ---
> tools/perf/util/session.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index f19348dddd55..e1014ab62c10 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1503,11 +1503,11 @@ static int machines__deliver_event(struct machines *machines,
> ++evlist->stats.nr_unknown_id;
> return 0;
> }
> - dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> if (machine == NULL) {
> ++evlist->stats.nr_unprocessable_samples;
> return 0;
> }
> + dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> return evlist__deliver_sample(evlist, tool, event, sample, evsel, machine);
> case PERF_RECORD_MMAP:
> return tool->mmap(tool, event, sample, machine);
>

Hi Ameer,

This mistake was made recently, but I don't think your change is the desired behavior.

It should be possible to dump stuff if machine is null. null or an empty string
should be passed to dump_sample(). It looks like some of the printfs still attempt to
show something in this case, although I didn't check them all.

James

2022-01-24 22:43:08

by Ameer Hamza

Subject: Re: [PATCH] perf session: check for null pointer before derefernce

On Mon, Jan 24, 2022 at 03:30:17PM +0000, James Clark wrote:
>
>
> On 24/01/2022 15:00, Ameer Hamza wrote:
> > Move null pointer check before dereferncing the variable
> >
> > Addresses-Coverity: 1497622 ("Derereference before null check")
> >
> > Signed-off-by: Ameer Hamza <[email protected]>
> > ---
> > tools/perf/util/session.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index f19348dddd55..e1014ab62c10 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -1503,11 +1503,11 @@ static int machines__deliver_event(struct machines *machines,
> > ++evlist->stats.nr_unknown_id;
> > return 0;
> > }
> > - dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> > if (machine == NULL) {
> > ++evlist->stats.nr_unprocessable_samples;
> > return 0;
> > }
> > + dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> > return evlist__deliver_sample(evlist, tool, event, sample, evsel, machine);
> > case PERF_RECORD_MMAP:
> > return tool->mmap(tool, event, sample, machine);
> >
>
> Hi Ameer,
>
> This mistake was made recently, but I don't think your change is the desired behavior.
>
> It should be possible to dump stuff if machine is null. null or an empty string
> should be passed to dump_sample(). It looks like some of the printfs still attempt to
> show something in this case, although I didn't check them all.

Hi James,

Thank you for your response. I understand your point.

Do you think following changes would be ok?

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index f19348dddd55..210eeee3dd70 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1503,11 +1503,12 @@ static int machines__deliver_event(struct machines *machines,
++evlist->stats.nr_unknown_id;
return 0;
}
- dump_sample(evsel, event, sample, perf_env__arch(machine->env));
if (machine == NULL) {
++evlist->stats.nr_unprocessable_samples;
+ dump_sample(evsel, event, sample, perf_env__arch(NULL));
return 0;
}
+ dump_sample(evsel, event, sample, perf_env__arch(machine->env));
return evlist__deliver_sample(evlist, tool, event, sample, evsel, machine);
case PERF_RECORD_MMAP:
return tool->mmap(tool, event, sample, machine);

Thanks,
Hamza

2022-01-25 14:30:48

by James Clark

Subject: Re: [PATCH] perf session: check for null pointer before derefernce

On 24/01/2022 20:29, Ameer Hamza wrote:
> On Mon, Jan 24, 2022 at 03:30:17PM +0000, James Clark wrote:
>>
>>
>> On 24/01/2022 15:00, Ameer Hamza wrote:
>>> Move null pointer check before dereferncing the variable
>>>
>>> Addresses-Coverity: 1497622 ("Derereference before null check")
>>>
>>> Signed-off-by: Ameer Hamza <[email protected]>
>>> ---
>>> tools/perf/util/session.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>>> index f19348dddd55..e1014ab62c10 100644
>>> --- a/tools/perf/util/session.c
>>> +++ b/tools/perf/util/session.c
>>> @@ -1503,11 +1503,11 @@ static int machines__deliver_event(struct machines *machines,
>>> ++evlist->stats.nr_unknown_id;
>>> return 0;
>>> }
>>> - dump_sample(evsel, event, sample, perf_env__arch(machine->env));
>>> if (machine == NULL) {
>>> ++evlist->stats.nr_unprocessable_samples;
>>> return 0;
>>> }
>>> + dump_sample(evsel, event, sample, perf_env__arch(machine->env));
>>> return evlist__deliver_sample(evlist, tool, event, sample, evsel, machine);
>>> case PERF_RECORD_MMAP:
>>> return tool->mmap(tool, event, sample, machine);
>>>
>>
>> Hi Ameer,
>>
>> This mistake was made recently, but I don't think your change is the desired behavior.
>>
>> It should be possible to dump stuff if machine is null. null or an empty string
>> should be passed to dump_sample(). It looks like some of the printfs still attempt to
>> show something in this case, although I didn't check them all.
>
> Hi James,
>
> Thank you for your response. I understand your point.
>
> Do you think following changes would be ok?

Yep looks good. With that change:

Reviewed-by: James Clark <[email protected]>

>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index f19348dddd55..210eeee3dd70 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1503,11 +1503,12 @@ static int machines__deliver_event(struct machines *machines,
> ++evlist->stats.nr_unknown_id;
> return 0;
> }
> - dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> if (machine == NULL) {
> ++evlist->stats.nr_unprocessable_samples;
> + dump_sample(evsel, event, sample, perf_env__arch(NULL));
> return 0;
> }
> + dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> return evlist__deliver_sample(evlist, tool, event, sample, evsel, machine);
> case PERF_RECORD_MMAP:
> return tool->mmap(tool, event, sample, machine);
>
> Thanks,
> Hamza
>

2022-01-25 16:52:10

by Ameer Hamza

Subject: [PATCH v2] perf session: check for null pointer before derefernce

Move null pointer check before dereferncing the variable

Addresses-Coverity: 1497622 ("Derereference before null check")

Reviewed-by: James Clark <[email protected]>

Signed-off-by: Ameer Hamza <[email protected]>

---
v1 -> v2: It should be possible to dump stuff if machine is NULL.
---
tools/perf/util/session.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index f19348dddd55..210eeee3dd70 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1503,11 +1503,12 @@ static int machines__deliver_event(struct machines *machines,
++evlist->stats.nr_unknown_id;
return 0;
}
- dump_sample(evsel, event, sample, perf_env__arch(machine->env));
if (machine == NULL) {
++evlist->stats.nr_unprocessable_samples;
+ dump_sample(evsel, event, sample, perf_env__arch(NULL));
return 0;
}
+ dump_sample(evsel, event, sample, perf_env__arch(machine->env));
return evlist__deliver_sample(evlist, tool, event, sample, evsel, machine);
case PERF_RECORD_MMAP:
return tool->mmap(tool, event, sample, machine);
--
2.25.1

2022-02-06 17:45:29

by Arnaldo Carvalho de Melo

Subject: Re: [PATCH] perf session: check for null pointer before derefernce

Em Tue, Jan 25, 2022 at 09:35:49AM +0000, James Clark escreveu:
>
>
> On 24/01/2022 20:29, Ameer Hamza wrote:
> > On Mon, Jan 24, 2022 at 03:30:17PM +0000, James Clark wrote:
> >>
> >>
> >> On 24/01/2022 15:00, Ameer Hamza wrote:
> >>> Move null pointer check before dereferncing the variable
> >>>
> >>> Addresses-Coverity: 1497622 ("Derereference before null check")
> >>>
> >>> Signed-off-by: Ameer Hamza <[email protected]>
> >>> ---
> >>> tools/perf/util/session.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> >>> index f19348dddd55..e1014ab62c10 100644
> >>> --- a/tools/perf/util/session.c
> >>> +++ b/tools/perf/util/session.c
> >>> @@ -1503,11 +1503,11 @@ static int machines__deliver_event(struct machines *machines,
> >>> ++evlist->stats.nr_unknown_id;
> >>> return 0;
> >>> }
> >>> - dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> >>> if (machine == NULL) {
> >>> ++evlist->stats.nr_unprocessable_samples;
> >>> return 0;
> >>> }
> >>> + dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> >>> return evlist__deliver_sample(evlist, tool, event, sample, evsel, machine);
> >>> case PERF_RECORD_MMAP:
> >>> return tool->mmap(tool, event, sample, machine);
> >>>
> >>
> >> Hi Ameer,
> >>
> >> This mistake was made recently, but I don't think your change is the desired behavior.
> >>
> >> It should be possible to dump stuff if machine is null. null or an empty string
> >> should be passed to dump_sample(). It looks like some of the printfs still attempt to
> >> show something in this case, although I didn't check them all.
> >
> > Hi James,
> >
> > Thank you for your response. I understand your point.
> >
> > Do you think following changes would be ok?
>
> Yep looks good. With that change:
>
> Reviewed-by: James Clark <[email protected]>

Thanks, applied.

- Arnaldo


> >
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index f19348dddd55..210eeee3dd70 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -1503,11 +1503,12 @@ static int machines__deliver_event(struct machines *machines,
> > ++evlist->stats.nr_unknown_id;
> > return 0;
> > }
> > - dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> > if (machine == NULL) {
> > ++evlist->stats.nr_unprocessable_samples;
> > + dump_sample(evsel, event, sample, perf_env__arch(NULL));
> > return 0;
> > }
> > + dump_sample(evsel, event, sample, perf_env__arch(machine->env));
> > return evlist__deliver_sample(evlist, tool, event, sample, evsel, machine);
> > case PERF_RECORD_MMAP:
> > return tool->mmap(tool, event, sample, machine);
> >
> > Thanks,
> > Hamza
> >

--

- Arnaldo