| Message ID | 20190702103420.27540-12-leo.yan@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | perf: Fix errors detected by Smatch | expand |
Hi Leo, On Tue, Jul 02, 2019 at 06:34:20PM +0800, Leo Yan wrote: > Based on the following report from Smatch, fix the potential > NULL pointer dereference check. > > tools/perf/util/cs-etm.c:2545 > cs_etm__process_auxtrace_info() error: we previously assumed > 'session->itrace_synth_opts' could be null (see line 2541) > > tools/perf/util/cs-etm.c > 2541 if (session->itrace_synth_opts && session->itrace_synth_opts->set) { > 2542 etm->synth_opts = *session->itrace_synth_opts; > 2543 } else { > 2544 itrace_synth_opts__set_default(&etm->synth_opts, > 2545 session->itrace_synth_opts->default_no_sample); > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > 2546 etm->synth_opts.callchain = false; > 2547 } > > To dismiss the potential NULL pointer dereference, this patch validates > the pointer 'session->itrace_synth_opts' before access its elements. > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > tools/perf/util/cs-etm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index 0c7776b51045..b79df56eb9df 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -2540,7 +2540,7 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > > if (session->itrace_synth_opts && session->itrace_synth_opts->set) { > etm->synth_opts = *session->itrace_synth_opts; > - } else { > + } else if (session->itrace_synth_opts) { This will work but we end up checking session->itrace_synth_opts twice. I suggest to check it once and then process with the if/else if valid: if (session->itrace_synth_opts) { if (session->itrace_synth_opts->set) { ... } else { ... } } Thanks, Mathieu > itrace_synth_opts__set_default(&etm->synth_opts, > session->itrace_synth_opts->default_no_sample); > etm->synth_opts.callchain = false; > -- > 2.17.1 >
Hi Mathieu, On Tue, Jul 02, 2019 at 11:03:06AM -0600, Mathieu Poirier wrote: > Hi Leo, > > On Tue, Jul 02, 2019 at 06:34:20PM +0800, Leo Yan wrote: > > Based on the following report from Smatch, fix the potential > > NULL pointer dereference check. > > > > tools/perf/util/cs-etm.c:2545 > > cs_etm__process_auxtrace_info() error: we previously assumed > > 'session->itrace_synth_opts' could be null (see line 2541) > > > > tools/perf/util/cs-etm.c > > 2541 if (session->itrace_synth_opts && session->itrace_synth_opts->set) { > > 2542 etm->synth_opts = *session->itrace_synth_opts; > > 2543 } else { > > 2544 itrace_synth_opts__set_default(&etm->synth_opts, > > 2545 session->itrace_synth_opts->default_no_sample); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > > 2546 etm->synth_opts.callchain = false; > > 2547 } > > > > To dismiss the potential NULL pointer dereference, this patch validates > > the pointer 'session->itrace_synth_opts' before access its elements. > > > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > > --- > > tools/perf/util/cs-etm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > > index 0c7776b51045..b79df56eb9df 100644 > > --- a/tools/perf/util/cs-etm.c > > +++ b/tools/perf/util/cs-etm.c > > @@ -2540,7 +2540,7 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > > > > if (session->itrace_synth_opts && session->itrace_synth_opts->set) { > > etm->synth_opts = *session->itrace_synth_opts; > > - } else { > > + } else if (session->itrace_synth_opts) { > > This will work but we end up checking session->itrace_synth_opts twice. I > suggest to check it once and then process with the if/else if valid: > > if (session->itrace_synth_opts) { > if (session->itrace_synth_opts->set) { > ... > } else { > ... > } > } Thanks for reviewing. As discussed with Adrian in another email for intel-pt, it's safe to remove NULL checking for session->itrace_synth_opts after I reviewed the code for report/script/inject. So I'm planning to apply the same change for cs-etm code. If you have concern for this, please let me know. Thanks, Leo Yan > > itrace_synth_opts__set_default(&etm->synth_opts, > > session->itrace_synth_opts->default_no_sample); > > etm->synth_opts.callchain = false; > > -- > > 2.17.1 > >
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 0c7776b51045..b79df56eb9df 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -2540,7 +2540,7 @@ int cs_etm__process_auxtrace_info(union perf_event *event, if (session->itrace_synth_opts && session->itrace_synth_opts->set) { etm->synth_opts = *session->itrace_synth_opts; - } else { + } else if (session->itrace_synth_opts) { itrace_synth_opts__set_default(&etm->synth_opts, session->itrace_synth_opts->default_no_sample); etm->synth_opts.callchain = false;
Based on the following report from Smatch, fix the potential NULL pointer dereference check. tools/perf/util/cs-etm.c:2545 cs_etm__process_auxtrace_info() error: we previously assumed 'session->itrace_synth_opts' could be null (see line 2541) tools/perf/util/cs-etm.c 2541 if (session->itrace_synth_opts && session->itrace_synth_opts->set) { 2542 etm->synth_opts = *session->itrace_synth_opts; 2543 } else { 2544 itrace_synth_opts__set_default(&etm->synth_opts, 2545 session->itrace_synth_opts->default_no_sample); ^^^^^^^^^^^^^^^^^^^^^^^^^^ 2546 etm->synth_opts.callchain = false; 2547 } To dismiss the potential NULL pointer dereference, this patch validates the pointer 'session->itrace_synth_opts' before access its elements. Signed-off-by: Leo Yan <leo.yan@linaro.org> --- tools/perf/util/cs-etm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.17.1