Re: [PATCH v1 11/11] perf cs-etm: Smatch: Fix potential NULL pointer dereference

[Mathieu Poirier]

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....@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
>