[PATCH v1 00/11] perf: Fix errors detected by Smatch

[PATCH v1 00/11] perf: Fix errors detected by Smatch

[Leo Yan]

When I used static checker Smatch for perf building, the main target is
to check if there have any potential issues in Arm cs-etm code.  So
finally I get many reporting for errors/warnings.

I used below command for using static checker with perf building:

  # make VF=1 CORESIGHT=1 -C tools/perf/ \
    CHECK="/root/Work/smatch/smatch --full-path" \
    CC=/root/Work/smatch/cgcc | tee smatch_reports.txt

I reviewed the errors one by one, if I understood some of these errors
so changed the code as I can, this patch set is the working result; but
still leave some errors due to I don't know what's the best way to fix
it.  There also have many inconsistent indenting warnings.  So I firstly
send out this patch set and let's see what's the feedback from public
reviewing.

Leo Yan (11):
  perf report: Smatch: Fix potential NULL pointer dereference
  perf stat: Smatch: Fix use-after-freed pointer
  perf top: Smatch: Fix potential NULL pointer dereference
  perf annotate: Smatch: Fix dereferencing freed memory
  perf trace: Smatch: Fix potential NULL pointer dereference
  perf hists: Smatch: Fix potential NULL pointer dereference
  perf map: Smatch: Fix potential NULL pointer dereference
  perf session: Smatch: Fix potential NULL pointer dereference
  perf intel-bts: Smatch: Fix potential NULL pointer dereference
  perf intel-pt: Smatch: Fix potential NULL pointer dereference
  perf cs-etm: Smatch: Fix potential NULL pointer dereference

 tools/perf/builtin-report.c    |  4 ++--
 tools/perf/builtin-stat.c      |  2 +-
 tools/perf/builtin-top.c       |  8 ++++++--
 tools/perf/builtin-trace.c     |  5 +++--
 tools/perf/ui/browsers/hists.c | 13 +++++++++----
 tools/perf/util/annotate.c     |  6 ++----
 tools/perf/util/cs-etm.c       |  2 +-
 tools/perf/util/intel-bts.c    |  5 ++---
 tools/perf/util/intel-pt.c     |  5 ++---
 tools/perf/util/map.c          |  7 +++++--
 tools/perf/util/session.c      |  3 +++
 11 files changed, 36 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH v1 01/11] perf report: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

Based on the following report from Smatch, fix the potential
NULL pointer dereference check.

  tools/perf/builtin-report.c:304
  process_read_event() error: we previously assumed 'evsel' could be null (see line 301)

tools/perf/builtin-report.c
301                 const char *name = evsel ? perf_evsel__name(evsel) : "unknown";
302                 int err = perf_read_values_add_value(&rep->show_threads_values,
303                                            event->read.pid, event->read.tid,
304                                            evsel->idx,
                                               ^^^^^^^
305                                            name,
306                                            event->read.value);

This patch checks if 'evsel' is NULL pointer then pass UINT64_MAX as idx
parameter.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/builtin-report.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 91c40808380d..a894ce7cd04e 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -299,10 +299,10 @@ static int process_read_event(struct perf_tool *tool,
 
 	if (rep->show_threads) {
 		const char *name = evsel ? perf_evsel__name(evsel) : "unknown";
+		int idx = evsel ? evsel->idx : INT_MAX;
 		int err = perf_read_values_add_value(&rep->show_threads_values,
 					   event->read.pid, event->read.tid,
-					   evsel->idx,
-					   name,
+					   idx, name,
 					   event->read.value);
 
 		if (err)
-- 
2.17.1

[PATCH v1 02/11] perf stat: Smatch: Fix use-after-freed pointer

[Leo Yan]

Based on the following report from Smatch, fix the use-after-freed
pointer.

  tools/perf/builtin-stat.c:1353
  add_default_attributes() warn: passing freed memory 'str'.

The pointer 'str' has been freed but later it is still passed into the
function parse_events_print_error().  This patch fixes this
use-after-freed issue.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/builtin-stat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 8a35fc5a7281..de0f6d0e96a2 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1349,8 +1349,8 @@ static int add_default_attributes(void)
 				fprintf(stderr,
 					"Cannot set up top down events %s: %d\n",
 					str, err);
-				free(str);
 				parse_events_print_error(&errinfo, str);
+				free(str);
 				return -1;
 			}
 		} else {
-- 
2.17.1

[PATCH v1 03/11] perf top: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

Based on the following report from Smatch, fix the potential
NULL pointer dereference check.

  tools/perf/builtin-top.c:109
  perf_top__parse_source() warn: variable dereferenced before check 'he'
  (see line 103)

  tools/perf/builtin-top.c:233
  perf_top__show_details() warn: variable dereferenced before check 'he'
  (see line 228)

tools/perf/builtin-top.c
101 static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
102 {
103         struct perf_evsel *evsel = hists_to_evsel(he->hists);
                                                      ^^^^
104         struct symbol *sym;
105         struct annotation *notes;
106         struct map *map;
107         int err = -1;
108
109         if (!he || !he->ms.sym)
110                 return -1;

This patch moves the values assignment after validating pointer 'he'.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/builtin-top.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 12b6b15a9675..13234c322981 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -100,7 +100,7 @@ static void perf_top__resize(struct perf_top *top)
 
 static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
 {
-	struct perf_evsel *evsel = hists_to_evsel(he->hists);
+	struct perf_evsel *evsel;
 	struct symbol *sym;
 	struct annotation *notes;
 	struct map *map;
@@ -109,6 +109,8 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
 	if (!he || !he->ms.sym)
 		return -1;
 
+	evsel = hists_to_evsel(he->hists);
+
 	sym = he->ms.sym;
 	map = he->ms.map;
 
@@ -225,7 +227,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 static void perf_top__show_details(struct perf_top *top)
 {
 	struct hist_entry *he = top->sym_filter_entry;
-	struct perf_evsel *evsel = hists_to_evsel(he->hists);
+	struct perf_evsel *evsel;
 	struct annotation *notes;
 	struct symbol *symbol;
 	int more;
@@ -233,6 +235,8 @@ static void perf_top__show_details(struct perf_top *top)
 	if (!he)
 		return;
 
+	evsel = hists_to_evsel(he->hists);
+
 	symbol = he->ms.sym;
 	notes = symbol__annotation(symbol);
 
-- 
2.17.1

[PATCH v1 04/11] perf annotate: Smatch: Fix dereferencing freed memory

[Leo Yan]

Based on the following report from Smatch, fix the potential
dereferencing freed memory check.

  tools/perf/util/annotate.c:1125
  disasm_line__parse() error: dereferencing freed memory 'namep'

tools/perf/util/annotate.c
1100 static int disasm_line__parse(char *line, const char **namep, char **rawp)
1101 {
1102         char tmp, *name = ltrim(line);

[...]

1114         *namep = strdup(name);
1115
1116         if (*namep == NULL)
1117                 goto out_free_name;

[...]

1124 out_free_name:
1125         free((void *)namep);
                          ^^^^^
1126         *namep = NULL;
             ^^^^^^
1127         return -1;
1128 }

If strdup() fails to allocate memory space for *namep, we don't need to
free memory with pointer 'namep', which is resident in data structure
disasm_line::ins::name; and *namep is NULL pointer for this failure, so
it's pointless to assign NULL to *namep again.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/annotate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index c8ce13419d9b..b8dfcfe08bb1 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1113,16 +1113,14 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
 	*namep = strdup(name);
 
 	if (*namep == NULL)
-		goto out_free_name;
+		goto out;
 
 	(*rawp)[0] = tmp;
 	*rawp = ltrim(*rawp);
 
 	return 0;
 
-out_free_name:
-	free((void *)namep);
-	*namep = NULL;
+out:
 	return -1;
 }
 
-- 
2.17.1

[PATCH v1 05/11] perf trace: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

Based on the following report from Smatch, fix the potential
NULL pointer dereference check.

  tools/perf/builtin-trace.c:1044
  thread_trace__new() error: we previously assumed 'ttrace' could be
  null (see line 1041).

tools/perf/builtin-trace.c
1037 static struct thread_trace *thread_trace__new(void)
1038 {
1039         struct thread_trace *ttrace =  zalloc(sizeof(struct thread_trace));
1040
1041         if (ttrace)
1042                 ttrace->files.max = -1;
1043
1044         ttrace->syscall_stats = intlist__new(NULL);
             ^^^^^^^^
1045
1046         return ttrace;
1047 }

This patch directly returns NULL when fail to allocate memory for
ttrace; this can avoid potential NULL pointer dereference.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/builtin-trace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index f3532b081b31..874d78890c60 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1038,9 +1038,10 @@ static struct thread_trace *thread_trace__new(void)
 {
 	struct thread_trace *ttrace =  zalloc(sizeof(struct thread_trace));
 
-	if (ttrace)
-		ttrace->files.max = -1;
+	if (ttrace == NULL)
+		return NULL;
 
+	ttrace->files.max = -1;
 	ttrace->syscall_stats = intlist__new(NULL);
 
 	return ttrace;
-- 
2.17.1

[PATCH v1 06/11] perf hists: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

Based on the following report from Smatch, fix the potential
NULL pointer dereference check.

  tools/perf/ui/browsers/hists.c:641
  hist_browser__run() error: we previously assumed 'hbt' could be
  null (see line 625)

  tools/perf/ui/browsers/hists.c:3088
  perf_evsel__hists_browse() error: we previously assumed
  'browser->he_selection' could be null (see line 2902)

  tools/perf/ui/browsers/hists.c:3272
  perf_evsel_menu__run() error: we previously assumed 'hbt' could be
  null (see line 3260)

This patch firstly validating the pointers before access them, so can
fix potential NULL pointer dereference.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/ui/browsers/hists.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 3421ecbdd3f0..2ba33040ddd8 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -638,7 +638,9 @@ int hist_browser__run(struct hist_browser *browser, const char *help,
 		switch (key) {
 		case K_TIMER: {
 			u64 nr_entries;
-			hbt->timer(hbt->arg);
+
+			if (hbt)
+				hbt->timer(hbt->arg);
 
 			if (hist_browser__has_filter(browser) ||
 			    symbol_conf.report_hierarchy)
@@ -2819,7 +2821,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 {
 	struct hists *hists = evsel__hists(evsel);
 	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env, annotation_opts);
-	struct branch_info *bi;
+	struct branch_info *bi = NULL;
 #define MAX_OPTIONS  16
 	char *options[MAX_OPTIONS];
 	struct popup_action actions[MAX_OPTIONS];
@@ -3085,7 +3087,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			goto skip_annotation;
 
 		if (sort__mode == SORT_MODE__BRANCH) {
-			bi = browser->he_selection->branch_info;
+
+			if (browser->he_selection)
+				bi = browser->he_selection->branch_info;
 
 			if (bi == NULL)
 				goto skip_annotation;
@@ -3269,7 +3273,8 @@ static int perf_evsel_menu__run(struct perf_evsel_menu *menu,
 
 		switch (key) {
 		case K_TIMER:
-			hbt->timer(hbt->arg);
+			if (hbt)
+				hbt->timer(hbt->arg);
 
 			if (!menu->lost_events_warned &&
 			    menu->lost_events &&
-- 
2.17.1

[PATCH v1 07/11] perf map: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

Based on the following report from Smatch, fix the potential
NULL pointer dereference check.

  tools/perf/util/map.c:479
  map__fprintf_srccode() error: we previously assumed 'state' could be
  null (see line 466)

tools/perf/util/map.c
465         /* Avoid redundant printing */
466         if (state &&
467             state->srcfile &&
468             !strcmp(state->srcfile, srcfile) &&
469             state->line == line) {
470                 free(srcfile);
471                 return 0;
472         }
473
474         srccode = find_sourceline(srcfile, line, &len);
475         if (!srccode)
476                 goto out_free_line;
477
478         ret = fprintf(fp, "|%-8d %.*s", line, len, srccode);
479         state->srcfile = srcfile;
            ^^^^^^^
480         state->line = line;
            ^^^^^^^

This patch validates 'state' pointer before access its elements.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/map.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 6fce983c6115..5f87975d2562 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -476,8 +476,11 @@ int map__fprintf_srccode(struct map *map, u64 addr,
 		goto out_free_line;
 
 	ret = fprintf(fp, "|%-8d %.*s", line, len, srccode);
-	state->srcfile = srcfile;
-	state->line = line;
+
+	if (state) {
+		state->srcfile = srcfile;
+		state->line = line;
+	}
 	return ret;
 
 out_free_line:
-- 
2.17.1

[PATCH v1 08/11] perf session: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

Based on the following report from Smatch, fix the potential
NULL pointer dereference check.

  tools/perf/util/session.c:1252
  dump_read() error: we previously assumed 'evsel' could be null
  (see line 1249)

tools/perf/util/session.c
1240 static void dump_read(struct perf_evsel *evsel, union perf_event *event)
1241 {
1242         struct read_event *read_event = &event->read;
1243         u64 read_format;
1244
1245         if (!dump_trace)
1246                 return;
1247
1248         printf(": %d %d %s %" PRIu64 "\n", event->read.pid, event->read.tid,
1249                evsel ? perf_evsel__name(evsel) : "FAIL",
1250                event->read.value);
1251
1252         read_format = evsel->attr.read_format;
                           ^^^^^^^

'evsel' could be NULL pointer, for this case this patch directly bails
out without dumping read_event.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/session.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 54cf163347f7..2e61dd6a3574 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1249,6 +1249,9 @@ static void dump_read(struct perf_evsel *evsel, union perf_event *event)
 	       evsel ? perf_evsel__name(evsel) : "FAIL",
 	       event->read.value);
 
+	if (!evsel)
+		return;
+
 	read_format = evsel->attr.read_format;
 
 	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
-- 
2.17.1

[PATCH v1 09/11] perf intel-bts: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

Based on the following report from Smatch, fix the potential
NULL pointer dereference check.

  tools/perf/util/intel-bts.c:898
  intel_bts_process_auxtrace_info() error: we previously assumed
  'session->itrace_synth_opts' could be null (see line 894)

  tools/perf/util/intel-bts.c:899
  intel_bts_process_auxtrace_info() warn: variable dereferenced before
  check 'session->itrace_synth_opts' (see line 898)

tools/perf/util/intel-bts.c
894         if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
895                 bts->synth_opts = *session->itrace_synth_opts;
896         } else {
897                 itrace_synth_opts__set_default(&bts->synth_opts,
898                                 session->itrace_synth_opts->default_no_sample);
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^
899                 if (session->itrace_synth_opts)
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^
900                         bts->synth_opts.thread_stack =
901                                 session->itrace_synth_opts->thread_stack;
902         }

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/intel-bts.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
index e32dbffebb2f..332e647fecaa 100644
--- a/tools/perf/util/intel-bts.c
+++ b/tools/perf/util/intel-bts.c
@@ -893,11 +893,10 @@ int intel_bts_process_auxtrace_info(union perf_event *event,
 
 	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
 		bts->synth_opts = *session->itrace_synth_opts;
-	} else {
+	} else if (session->itrace_synth_opts) {
 		itrace_synth_opts__set_default(&bts->synth_opts,
 				session->itrace_synth_opts->default_no_sample);
-		if (session->itrace_synth_opts)
-			bts->synth_opts.thread_stack =
+		bts->synth_opts.thread_stack =
 				session->itrace_synth_opts->thread_stack;
 	}
 
-- 
2.17.1

[PATCH v1 10/11] perf intel-pt: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

Based on the following report from Smatch, fix the potential
NULL pointer dereference check.

  tools/perf/util/intel-pt.c:3200
  intel_pt_process_auxtrace_info() error: we previously assumed
  'session->itrace_synth_opts' could be null (see line 3196)

  tools/perf/util/intel-pt.c:3206
  intel_pt_process_auxtrace_info() warn: variable dereferenced before
  check 'session->itrace_synth_opts' (see line 3200)

tools/perf/util/intel-pt.c
3196         if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
3197                 pt->synth_opts = *session->itrace_synth_opts;
3198         } else {
3199                 itrace_synth_opts__set_default(&pt->synth_opts,
3200                                 session->itrace_synth_opts->default_no_sample);
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
3201                 if (!session->itrace_synth_opts->default_no_sample &&
3202                     !session->itrace_synth_opts->inject) {
3203                         pt->synth_opts.branches = false;
3204                         pt->synth_opts.callchain = true;
3205                 }
3206                 if (session->itrace_synth_opts)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^
3207                         pt->synth_opts.thread_stack =
3208                                 session->itrace_synth_opts->thread_stack;
3209         }

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/intel-pt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 550db6e77968..88b567bdf1f9 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -3195,7 +3195,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
 
 	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
 		pt->synth_opts = *session->itrace_synth_opts;
-	} else {
+	} else if (session->itrace_synth_opts) {
 		itrace_synth_opts__set_default(&pt->synth_opts,
 				session->itrace_synth_opts->default_no_sample);
 		if (!session->itrace_synth_opts->default_no_sample &&
@@ -3203,8 +3203,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
 			pt->synth_opts.branches = false;
 			pt->synth_opts.callchain = true;
 		}
-		if (session->itrace_synth_opts)
-			pt->synth_opts.thread_stack =
+		pt->synth_opts.thread_stack =
 				session->itrace_synth_opts->thread_stack;
 	}
 
-- 
2.17.1

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

[Leo Yan]

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) {
 		itrace_synth_opts__set_default(&etm->synth_opts,
 				session->itrace_synth_opts->default_no_sample);
 		etm->synth_opts.callchain = false;
-- 
2.17.1

Re: [PATCH v1 06/11] perf hists: Smatch: Fix potential NULL pointer dereference

[Jiri Olsa]

On Tue, Jul 02, 2019 at 06:34:15PM +0800, Leo Yan wrote:
> Based on the following report from Smatch, fix the potential
> NULL pointer dereference check.
> 
>   tools/perf/ui/browsers/hists.c:641
>   hist_browser__run() error: we previously assumed 'hbt' could be
>   null (see line 625)
> 
>   tools/perf/ui/browsers/hists.c:3088
>   perf_evsel__hists_browse() error: we previously assumed
>   'browser->he_selection' could be null (see line 2902)
> 
>   tools/perf/ui/browsers/hists.c:3272
>   perf_evsel_menu__run() error: we previously assumed 'hbt' could be
>   null (see line 3260)
> 
> This patch firstly validating the pointers before access them, so can
> fix potential NULL pointer dereference.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/ui/browsers/hists.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 3421ecbdd3f0..2ba33040ddd8 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -638,7 +638,9 @@ int hist_browser__run(struct hist_browser *browser, const char *help,
>  		switch (key) {
>  		case K_TIMER: {

not sure this can really happen, perhaps WARN_ON_ONCE(!hbt) would be
good in here

jirka

>  			u64 nr_entries;
> -			hbt->timer(hbt->arg);
> +
> +			if (hbt)
> +				hbt->timer(hbt->arg);
>  
>  			if (hist_browser__has_filter(browser) ||
>  			    symbol_conf.report_hierarchy)

SNIP

Re: [PATCH v1 10/11] perf intel-pt: Smatch: Fix potential NULL pointer dereference

[Adrian Hunter]

On 2/07/19 1:34 PM, Leo Yan wrote:
> Based on the following report from Smatch, fix the potential
> NULL pointer dereference check.

It never is NULL.  Remove the NULL test if you want:

-	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
+	if (session->itrace_synth_opts->set) {

But blindly making changes like below is questionable.

> 
>   tools/perf/util/intel-pt.c:3200
>   intel_pt_process_auxtrace_info() error: we previously assumed
>   'session->itrace_synth_opts' could be null (see line 3196)
> 
>   tools/perf/util/intel-pt.c:3206
>   intel_pt_process_auxtrace_info() warn: variable dereferenced before
>   check 'session->itrace_synth_opts' (see line 3200)
> 
> tools/perf/util/intel-pt.c
> 3196         if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> 3197                 pt->synth_opts = *session->itrace_synth_opts;
> 3198         } else {
> 3199                 itrace_synth_opts__set_default(&pt->synth_opts,
> 3200                                 session->itrace_synth_opts->default_no_sample);
>                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 3201                 if (!session->itrace_synth_opts->default_no_sample &&
> 3202                     !session->itrace_synth_opts->inject) {
> 3203                         pt->synth_opts.branches = false;
> 3204                         pt->synth_opts.callchain = true;
> 3205                 }
> 3206                 if (session->itrace_synth_opts)
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 3207                         pt->synth_opts.thread_stack =
> 3208                                 session->itrace_synth_opts->thread_stack;
> 3209         }
> 
> 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/intel-pt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index 550db6e77968..88b567bdf1f9 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -3195,7 +3195,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
>  
>  	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
>  		pt->synth_opts = *session->itrace_synth_opts;
> -	} else {
> +	} else if (session->itrace_synth_opts) {
>  		itrace_synth_opts__set_default(&pt->synth_opts,
>  				session->itrace_synth_opts->default_no_sample);
>  		if (!session->itrace_synth_opts->default_no_sample &&
> @@ -3203,8 +3203,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
>  			pt->synth_opts.branches = false;
>  			pt->synth_opts.callchain = true;
>  		}
> -		if (session->itrace_synth_opts)
> -			pt->synth_opts.thread_stack =
> +		pt->synth_opts.thread_stack =
>  				session->itrace_synth_opts->thread_stack;
>  	}
>  
> 

Re: [PATCH v1 00/11] perf: Fix errors detected by Smatch

[Jiri Olsa]

On Tue, Jul 02, 2019 at 06:34:09PM +0800, Leo Yan wrote:
> When I used static checker Smatch for perf building, the main target is
> to check if there have any potential issues in Arm cs-etm code.  So
> finally I get many reporting for errors/warnings.
> 
> I used below command for using static checker with perf building:
> 
>   # make VF=1 CORESIGHT=1 -C tools/perf/ \
>     CHECK="/root/Work/smatch/smatch --full-path" \
>     CC=/root/Work/smatch/cgcc | tee smatch_reports.txt
> 
> I reviewed the errors one by one, if I understood some of these errors
> so changed the code as I can, this patch set is the working result; but
> still leave some errors due to I don't know what's the best way to fix
> it.  There also have many inconsistent indenting warnings.  So I firstly
> send out this patch set and let's see what's the feedback from public
> reviewing.
> 
> Leo Yan (11):
>   perf report: Smatch: Fix potential NULL pointer dereference
>   perf stat: Smatch: Fix use-after-freed pointer
>   perf top: Smatch: Fix potential NULL pointer dereference
>   perf annotate: Smatch: Fix dereferencing freed memory
>   perf trace: Smatch: Fix potential NULL pointer dereference
>   perf hists: Smatch: Fix potential NULL pointer dereference
>   perf map: Smatch: Fix potential NULL pointer dereference
>   perf session: Smatch: Fix potential NULL pointer dereference
>   perf intel-bts: Smatch: Fix potential NULL pointer dereference
>   perf intel-pt: Smatch: Fix potential NULL pointer dereference
>   perf cs-etm: Smatch: Fix potential NULL pointer dereference

from quick look it all looks good to me, nice tool ;-)

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> 
>  tools/perf/builtin-report.c    |  4 ++--
>  tools/perf/builtin-stat.c      |  2 +-
>  tools/perf/builtin-top.c       |  8 ++++++--
>  tools/perf/builtin-trace.c     |  5 +++--
>  tools/perf/ui/browsers/hists.c | 13 +++++++++----
>  tools/perf/util/annotate.c     |  6 ++----
>  tools/perf/util/cs-etm.c       |  2 +-
>  tools/perf/util/intel-bts.c    |  5 ++---
>  tools/perf/util/intel-pt.c     |  5 ++---
>  tools/perf/util/map.c          |  7 +++++--
>  tools/perf/util/session.c      |  3 +++
>  11 files changed, 36 insertions(+), 24 deletions(-)
> 
> -- 
> 2.17.1
> 

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

Re: [PATCH v1 10/11] perf intel-pt: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

Hi Adrian,

On Tue, Jul 02, 2019 at 02:07:40PM +0300, Adrian Hunter wrote:
> On 2/07/19 1:34 PM, Leo Yan wrote:
> > Based on the following report from Smatch, fix the potential
> > NULL pointer dereference check.
> 
> It never is NULL.  Remove the NULL test if you want:
> 
> -	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> +	if (session->itrace_synth_opts->set) {
> 
> But blindly making changes like below is questionable.

Thanks for suggestions.

I checked report and script commands, as you said, both command will
always set session->itrace_synth_opts.  For these two commands, we can
safely remove the NULL test.

Because perf tool contains many sub commands, so I don't have much
confidence it's very safe to remove the NULL test for all cases; e.g.
there have cases which will process aux trace buffer but without
itrace options; for this case, session->itrace_synth_opts might be NULL.

For either way (remove NULL test or keep NULL test), I don't want to
introduce regression and extra efforts by my patch.  So want to double
confirm with you for this :)

Thanks,
Leo Yan

> >   tools/perf/util/intel-pt.c:3200
> >   intel_pt_process_auxtrace_info() error: we previously assumed
> >   'session->itrace_synth_opts' could be null (see line 3196)
> > 
> >   tools/perf/util/intel-pt.c:3206
> >   intel_pt_process_auxtrace_info() warn: variable dereferenced before
> >   check 'session->itrace_synth_opts' (see line 3200)
> > 
> > tools/perf/util/intel-pt.c
> > 3196         if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> > 3197                 pt->synth_opts = *session->itrace_synth_opts;
> > 3198         } else {
> > 3199                 itrace_synth_opts__set_default(&pt->synth_opts,
> > 3200                                 session->itrace_synth_opts->default_no_sample);
> >                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 3201                 if (!session->itrace_synth_opts->default_no_sample &&
> > 3202                     !session->itrace_synth_opts->inject) {
> > 3203                         pt->synth_opts.branches = false;
> > 3204                         pt->synth_opts.callchain = true;
> > 3205                 }
> > 3206                 if (session->itrace_synth_opts)
> >                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 3207                         pt->synth_opts.thread_stack =
> > 3208                                 session->itrace_synth_opts->thread_stack;
> > 3209         }
> > 
> > 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/intel-pt.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> > index 550db6e77968..88b567bdf1f9 100644
> > --- a/tools/perf/util/intel-pt.c
> > +++ b/tools/perf/util/intel-pt.c
> > @@ -3195,7 +3195,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
> >  
> >  	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> >  		pt->synth_opts = *session->itrace_synth_opts;
> > -	} else {
> > +	} else if (session->itrace_synth_opts) {
> >  		itrace_synth_opts__set_default(&pt->synth_opts,
> >  				session->itrace_synth_opts->default_no_sample);
> >  		if (!session->itrace_synth_opts->default_no_sample &&
> > @@ -3203,8 +3203,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
> >  			pt->synth_opts.branches = false;
> >  			pt->synth_opts.callchain = true;
> >  		}
> > -		if (session->itrace_synth_opts)
> > -			pt->synth_opts.thread_stack =
> > +		pt->synth_opts.thread_stack =
> >  				session->itrace_synth_opts->thread_stack;
> >  	}
> >  
> > 
> 

Re: [PATCH v1 00/11] perf: Fix errors detected by Smatch

[Leo Yan]

On Tue, Jul 02, 2019 at 01:07:43PM +0200, Jiri Olsa wrote:
> On Tue, Jul 02, 2019 at 06:34:09PM +0800, Leo Yan wrote:
> > When I used static checker Smatch for perf building, the main target is
> > to check if there have any potential issues in Arm cs-etm code.  So
> > finally I get many reporting for errors/warnings.
> > 
> > I used below command for using static checker with perf building:
> > 
> >   # make VF=1 CORESIGHT=1 -C tools/perf/ \
> >     CHECK="/root/Work/smatch/smatch --full-path" \
> >     CC=/root/Work/smatch/cgcc | tee smatch_reports.txt
> > 
> > I reviewed the errors one by one, if I understood some of these errors
> > so changed the code as I can, this patch set is the working result; but
> > still leave some errors due to I don't know what's the best way to fix
> > it.  There also have many inconsistent indenting warnings.  So I firstly
> > send out this patch set and let's see what's the feedback from public
> > reviewing.
> > 
> > Leo Yan (11):
> >   perf report: Smatch: Fix potential NULL pointer dereference
> >   perf stat: Smatch: Fix use-after-freed pointer
> >   perf top: Smatch: Fix potential NULL pointer dereference
> >   perf annotate: Smatch: Fix dereferencing freed memory
> >   perf trace: Smatch: Fix potential NULL pointer dereference
> >   perf hists: Smatch: Fix potential NULL pointer dereference
> >   perf map: Smatch: Fix potential NULL pointer dereference
> >   perf session: Smatch: Fix potential NULL pointer dereference
> >   perf intel-bts: Smatch: Fix potential NULL pointer dereference
> >   perf intel-pt: Smatch: Fix potential NULL pointer dereference
> >   perf cs-etm: Smatch: Fix potential NULL pointer dereference
> 
> from quick look it all looks good to me, nice tool ;-)
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Thanks for reviewing, Jiri.

@Arnaldo, Just want to check, will you firstly pick up 01~05, 07,
08/11 patches if you think they are okay?  Or you want to wait me to
spin new patch set with all patches after I gather all comments?

Thanks,
Leo Yan

> >  tools/perf/builtin-report.c    |  4 ++--
> >  tools/perf/builtin-stat.c      |  2 +-
> >  tools/perf/builtin-top.c       |  8 ++++++--
> >  tools/perf/builtin-trace.c     |  5 +++--
> >  tools/perf/ui/browsers/hists.c | 13 +++++++++----
> >  tools/perf/util/annotate.c     |  6 ++----
> >  tools/perf/util/cs-etm.c       |  2 +-
> >  tools/perf/util/intel-bts.c    |  5 ++---
> >  tools/perf/util/intel-pt.c     |  5 ++---
> >  tools/perf/util/map.c          |  7 +++++--
> >  tools/perf/util/session.c      |  3 +++
> >  11 files changed, 36 insertions(+), 24 deletions(-)
> > 
> > -- 
> > 2.17.1
> > 

Re: [PATCH v1 10/11] perf intel-pt: Smatch: Fix potential NULL pointer dereference

[Adrian Hunter]

On 3/07/19 4:35 AM, Leo Yan wrote:
> Hi Adrian,
> 
> On Tue, Jul 02, 2019 at 02:07:40PM +0300, Adrian Hunter wrote:
>> On 2/07/19 1:34 PM, Leo Yan wrote:
>>> Based on the following report from Smatch, fix the potential
>>> NULL pointer dereference check.
>>
>> It never is NULL.  Remove the NULL test if you want:
>>
>> -	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
>> +	if (session->itrace_synth_opts->set) {
>>
>> But blindly making changes like below is questionable.
> 
> Thanks for suggestions.
> 
> I checked report and script commands, as you said, both command will
> always set session->itrace_synth_opts.  For these two commands, we can
> safely remove the NULL test.
> 
> Because perf tool contains many sub commands, so I don't have much
> confidence it's very safe to remove the NULL test for all cases; e.g.
> there have cases which will process aux trace buffer but without
> itrace options; for this case, session->itrace_synth_opts might be NULL.
> 
> For either way (remove NULL test or keep NULL test), I don't want to
> introduce regression and extra efforts by my patch.  So want to double
> confirm with you for this :)

Yes, intel_pt_process_auxtrace_info() only gets called if a tool sets up the
tools->auxtrace_info() callback.  The tools that do that also set
session->itrace_synth_opts.

Re: [PATCH v1 10/11] perf intel-pt: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

On Wed, Jul 03, 2019 at 08:19:19AM +0300, Adrian Hunter wrote:
> On 3/07/19 4:35 AM, Leo Yan wrote:
> > Hi Adrian,
> > 
> > On Tue, Jul 02, 2019 at 02:07:40PM +0300, Adrian Hunter wrote:
> >> On 2/07/19 1:34 PM, Leo Yan wrote:
> >>> Based on the following report from Smatch, fix the potential
> >>> NULL pointer dereference check.
> >>
> >> It never is NULL.  Remove the NULL test if you want:
> >>
> >> -	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> >> +	if (session->itrace_synth_opts->set) {
> >>
> >> But blindly making changes like below is questionable.
> > 
> > Thanks for suggestions.
> > 
> > I checked report and script commands, as you said, both command will
> > always set session->itrace_synth_opts.  For these two commands, we can
> > safely remove the NULL test.
> > 
> > Because perf tool contains many sub commands, so I don't have much
> > confidence it's very safe to remove the NULL test for all cases; e.g.
> > there have cases which will process aux trace buffer but without
> > itrace options; for this case, session->itrace_synth_opts might be NULL.
> > 
> > For either way (remove NULL test or keep NULL test), I don't want to
> > introduce regression and extra efforts by my patch.  So want to double
> > confirm with you for this :)
> 
> Yes, intel_pt_process_auxtrace_info() only gets called if a tool sets up the
> tools->auxtrace_info() callback.  The tools that do that also set
> session->itrace_synth_opts.

Yes.

I also checked the another case for 'perf inject', just as you said,
it sets both inject->tool.auxtrace_info and session->itrace_synth_opts
if we use 'itrace' option.  So it's safe to remove the NULL test for
session->itrace_synth_opts.

Will follow your suggestion for new patches.  Thanks a lot for
confirmation and suggestions.

Thanks,
Leo Yan

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

[Leo Yan]

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

Re: [PATCH v1 10/11] perf intel-pt: Smatch: Fix potential NULL pointer dereference

[Daniel Thompson]

On Wed, Jul 03, 2019 at 09:35:54AM +0800, Leo Yan wrote:
> Hi Adrian,
> 
> On Tue, Jul 02, 2019 at 02:07:40PM +0300, Adrian Hunter wrote:
> > On 2/07/19 1:34 PM, Leo Yan wrote:
> > > Based on the following report from Smatch, fix the potential
> > > NULL pointer dereference check.
> > 
> > It never is NULL.  Remove the NULL test if you want:
> > 
> > -	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> > +	if (session->itrace_synth_opts->set) {
> > 
> > But blindly making changes like below is questionable.
> 
> Thanks for suggestions.
> 
> I checked report and script commands, as you said, both command will
> always set session->itrace_synth_opts.  For these two commands, we can
> safely remove the NULL test.
> 
> Because perf tool contains many sub commands, so I don't have much
> confidence it's very safe to remove the NULL test for all cases; e.g.
> there have cases which will process aux trace buffer but without
> itrace options; for this case, session->itrace_synth_opts might be NULL.
> 
> For either way (remove NULL test or keep NULL test), I don't want to
> introduce regression and extra efforts by my patch.  So want to double
> confirm with you for this :)

Review is useful to ensure the chosen solution is correct but
unless I missed something the non-regression reasoning here is easy
easy. In its original form and despite the check, the code will
always dereference session->itrace_synth_opts, therefore removing
the check cannot makes things worse.


Daniel.


PS Of course we do also have to check that
   itrace_synth_opts__set_default() isn't a macro... but it isn't.


> > >   tools/perf/util/intel-pt.c:3200
> > >   intel_pt_process_auxtrace_info() error: we previously assumed
> > >   'session->itrace_synth_opts' could be null (see line 3196)
> > > 
> > >   tools/perf/util/intel-pt.c:3206
> > >   intel_pt_process_auxtrace_info() warn: variable dereferenced before
> > >   check 'session->itrace_synth_opts' (see line 3200)
> > > 
> > > tools/perf/util/intel-pt.c
> > > 3196         if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> > > 3197                 pt->synth_opts = *session->itrace_synth_opts;
> > > 3198         } else {
> > > 3199                 itrace_synth_opts__set_default(&pt->synth_opts,
> > > 3200                                 session->itrace_synth_opts->default_no_sample);
> > >                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 3201                 if (!session->itrace_synth_opts->default_no_sample &&
> > > 3202                     !session->itrace_synth_opts->inject) {
> > > 3203                         pt->synth_opts.branches = false;
> > > 3204                         pt->synth_opts.callchain = true;
> > > 3205                 }
> > > 3206                 if (session->itrace_synth_opts)
> > >                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 3207                         pt->synth_opts.thread_stack =
> > > 3208                                 session->itrace_synth_opts->thread_stack;
> > > 3209         }
> > > 
> > > 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/intel-pt.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> > > index 550db6e77968..88b567bdf1f9 100644
> > > --- a/tools/perf/util/intel-pt.c
> > > +++ b/tools/perf/util/intel-pt.c
> > > @@ -3195,7 +3195,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
> > >  
> > >  	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> > >  		pt->synth_opts = *session->itrace_synth_opts;
> > > -	} else {
> > > +	} else if (session->itrace_synth_opts) {
> > >  		itrace_synth_opts__set_default(&pt->synth_opts,
> > >  				session->itrace_synth_opts->default_no_sample);
> > >  		if (!session->itrace_synth_opts->default_no_sample &&
> > > @@ -3203,8 +3203,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
> > >  			pt->synth_opts.branches = false;
> > >  			pt->synth_opts.callchain = true;
> > >  		}
> > > -		if (session->itrace_synth_opts)
> > > -			pt->synth_opts.thread_stack =
> > > +		pt->synth_opts.thread_stack =
> > >  				session->itrace_synth_opts->thread_stack;
> > >  	}
> > >  
> > > 
> > 

Re: [PATCH v1 10/11] perf intel-pt: Smatch: Fix potential NULL pointer dereference

[Leo Yan]

On Wed, Jul 03, 2019 at 11:00:32AM +0100, Daniel Thompson wrote:
> On Wed, Jul 03, 2019 at 09:35:54AM +0800, Leo Yan wrote:
> > Hi Adrian,
> > 
> > On Tue, Jul 02, 2019 at 02:07:40PM +0300, Adrian Hunter wrote:
> > > On 2/07/19 1:34 PM, Leo Yan wrote:
> > > > Based on the following report from Smatch, fix the potential
> > > > NULL pointer dereference check.
> > > 
> > > It never is NULL.  Remove the NULL test if you want:
> > > 
> > > -	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> > > +	if (session->itrace_synth_opts->set) {
> > > 
> > > But blindly making changes like below is questionable.
> > 
> > Thanks for suggestions.
> > 
> > I checked report and script commands, as you said, both command will
> > always set session->itrace_synth_opts.  For these two commands, we can
> > safely remove the NULL test.
> > 
> > Because perf tool contains many sub commands, so I don't have much
> > confidence it's very safe to remove the NULL test for all cases; e.g.
> > there have cases which will process aux trace buffer but without
> > itrace options; for this case, session->itrace_synth_opts might be NULL.
> > 
> > For either way (remove NULL test or keep NULL test), I don't want to
> > introduce regression and extra efforts by my patch.  So want to double
> > confirm with you for this :)
> 
> Review is useful to ensure the chosen solution is correct but
> unless I missed something the non-regression reasoning here is easy
> easy. In its original form and despite the check, the code will
> always dereference session->itrace_synth_opts, therefore removing
> the check cannot makes things worse.

Fair point and it's smart to connect with function
itrace_synth_opts__set_default(). :)

Thanks, Daniel.

> PS Of course we do also have to check that
>    itrace_synth_opts__set_default() isn't a macro... but it isn't.
> 
> 
> > > >   tools/perf/util/intel-pt.c:3200
> > > >   intel_pt_process_auxtrace_info() error: we previously assumed
> > > >   'session->itrace_synth_opts' could be null (see line 3196)
> > > > 
> > > >   tools/perf/util/intel-pt.c:3206
> > > >   intel_pt_process_auxtrace_info() warn: variable dereferenced before
> > > >   check 'session->itrace_synth_opts' (see line 3200)
> > > > 
> > > > tools/perf/util/intel-pt.c
> > > > 3196         if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> > > > 3197                 pt->synth_opts = *session->itrace_synth_opts;
> > > > 3198         } else {
> > > > 3199                 itrace_synth_opts__set_default(&pt->synth_opts,
> > > > 3200                                 session->itrace_synth_opts->default_no_sample);
> > > >                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > 3201                 if (!session->itrace_synth_opts->default_no_sample &&
> > > > 3202                     !session->itrace_synth_opts->inject) {
> > > > 3203                         pt->synth_opts.branches = false;
> > > > 3204                         pt->synth_opts.callchain = true;
> > > > 3205                 }
> > > > 3206                 if (session->itrace_synth_opts)
> > > >                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > 3207                         pt->synth_opts.thread_stack =
> > > > 3208                                 session->itrace_synth_opts->thread_stack;
> > > > 3209         }
> > > > 
> > > > 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/intel-pt.c | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> > > > index 550db6e77968..88b567bdf1f9 100644
> > > > --- a/tools/perf/util/intel-pt.c
> > > > +++ b/tools/perf/util/intel-pt.c
> > > > @@ -3195,7 +3195,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
> > > >  
> > > >  	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
> > > >  		pt->synth_opts = *session->itrace_synth_opts;
> > > > -	} else {
> > > > +	} else if (session->itrace_synth_opts) {
> > > >  		itrace_synth_opts__set_default(&pt->synth_opts,
> > > >  				session->itrace_synth_opts->default_no_sample);
> > > >  		if (!session->itrace_synth_opts->default_no_sample &&
> > > > @@ -3203,8 +3203,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
> > > >  			pt->synth_opts.branches = false;
> > > >  			pt->synth_opts.callchain = true;
> > > >  		}
> > > > -		if (session->itrace_synth_opts)
> > > > -			pt->synth_opts.thread_stack =
> > > > +		pt->synth_opts.thread_stack =
> > > >  				session->itrace_synth_opts->thread_stack;
> > > >  	}
> > > >  
> > > > 
> > > 

Re: [PATCH v1 00/11] perf: Fix errors detected by Smatch

[Arnaldo Carvalho de Melo]

Em Wed, Jul 03, 2019 at 09:48:08AM +0800, Leo Yan escreveu:
> On Tue, Jul 02, 2019 at 01:07:43PM +0200, Jiri Olsa wrote:
> > On Tue, Jul 02, 2019 at 06:34:09PM +0800, Leo Yan wrote:
> > > When I used static checker Smatch for perf building, the main target is
> > > to check if there have any potential issues in Arm cs-etm code.  So
> > > finally I get many reporting for errors/warnings.
> > > 
> > > I used below command for using static checker with perf building:
> > > 
> > >   # make VF=1 CORESIGHT=1 -C tools/perf/ \
> > >     CHECK="/root/Work/smatch/smatch --full-path" \
> > >     CC=/root/Work/smatch/cgcc | tee smatch_reports.txt
> > > 
> > > I reviewed the errors one by one, if I understood some of these errors
> > > so changed the code as I can, this patch set is the working result; but
> > > still leave some errors due to I don't know what's the best way to fix
> > > it.  There also have many inconsistent indenting warnings.  So I firstly
> > > send out this patch set and let's see what's the feedback from public
> > > reviewing.
> > > 
> > > Leo Yan (11):
> > >   perf report: Smatch: Fix potential NULL pointer dereference
> > >   perf stat: Smatch: Fix use-after-freed pointer
> > >   perf top: Smatch: Fix potential NULL pointer dereference
> > >   perf annotate: Smatch: Fix dereferencing freed memory
> > >   perf trace: Smatch: Fix potential NULL pointer dereference
> > >   perf hists: Smatch: Fix potential NULL pointer dereference
> > >   perf map: Smatch: Fix potential NULL pointer dereference
> > >   perf session: Smatch: Fix potential NULL pointer dereference
> > >   perf intel-bts: Smatch: Fix potential NULL pointer dereference
> > >   perf intel-pt: Smatch: Fix potential NULL pointer dereference
> > >   perf cs-etm: Smatch: Fix potential NULL pointer dereference
> > 
> > from quick look it all looks good to me, nice tool ;-)
> > 
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> 
> Thanks for reviewing, Jiri.
> 
> @Arnaldo, Just want to check, will you firstly pick up 01~05, 07,
> 08/11 patches if you think they are okay?  Or you want to wait me to
> spin new patch set with all patches after I gather all comments?

I'm picking up the uncontrovertial, will push to my perf/core branch,
continue from there, please.

- Arnaldo
 
> Thanks,
> Leo Yan
> 
> > >  tools/perf/builtin-report.c    |  4 ++--
> > >  tools/perf/builtin-stat.c      |  2 +-
> > >  tools/perf/builtin-top.c       |  8 ++++++--
> > >  tools/perf/builtin-trace.c     |  5 +++--
> > >  tools/perf/ui/browsers/hists.c | 13 +++++++++----
> > >  tools/perf/util/annotate.c     |  6 ++----
> > >  tools/perf/util/cs-etm.c       |  2 +-
> > >  tools/perf/util/intel-bts.c    |  5 ++---
> > >  tools/perf/util/intel-pt.c     |  5 ++---
> > >  tools/perf/util/map.c          |  7 +++++--
> > >  tools/perf/util/session.c      |  3 +++
> > >  11 files changed, 36 insertions(+), 24 deletions(-)
> > > 
> > > -- 
> > > 2.17.1
> > > 

-- 

- Arnaldo

Re: [PATCH v1 02/11] perf stat: Smatch: Fix use-after-freed pointer

[Arnaldo Carvalho de Melo]

Em Tue, Jul 02, 2019 at 06:34:11PM +0800, Leo Yan escreveu:
> Based on the following report from Smatch, fix the use-after-freed
> pointer.
> 
>   tools/perf/builtin-stat.c:1353
>   add_default_attributes() warn: passing freed memory 'str'.
> 
> The pointer 'str' has been freed but later it is still passed into the
> function parse_events_print_error().  This patch fixes this
> use-after-freed issue.

thanks, applied.
 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/builtin-stat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 8a35fc5a7281..de0f6d0e96a2 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1349,8 +1349,8 @@ static int add_default_attributes(void)
>  				fprintf(stderr,
>  					"Cannot set up top down events %s: %d\n",
>  					str, err);
> -				free(str);
>  				parse_events_print_error(&errinfo, str);
> +				free(str);
>  				return -1;
>  			}
>  		} else {
> -- 
> 2.17.1

-- 

- Arnaldo

Re: [PATCH v1 03/11] perf top: Smatch: Fix potential NULL pointer dereference

[Arnaldo Carvalho de Melo]

Em Tue, Jul 02, 2019 at 06:34:12PM +0800, Leo Yan escreveu:
> Based on the following report from Smatch, fix the potential
> NULL pointer dereference check.
> 
>   tools/perf/builtin-top.c:109
>   perf_top__parse_source() warn: variable dereferenced before check 'he'
>   (see line 103)
> 
>   tools/perf/builtin-top.c:233
>   perf_top__show_details() warn: variable dereferenced before check 'he'
>   (see line 228)
> 
> tools/perf/builtin-top.c
> 101 static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
> 102 {
> 103         struct perf_evsel *evsel = hists_to_evsel(he->hists);
>                                                       ^^^^
> 104         struct symbol *sym;
> 105         struct annotation *notes;
> 106         struct map *map;
> 107         int err = -1;
> 108
> 109         if (!he || !he->ms.sym)
> 110                 return -1;
> 
> This patch moves the values assignment after validating pointer 'he'.

Applied, thanks,

- Arnaldo

Re: [PATCH v1 04/11] perf annotate: Smatch: Fix dereferencing freed memory

[Arnaldo Carvalho de Melo]

Em Tue, Jul 02, 2019 at 06:34:13PM +0800, Leo Yan escreveu:
> Based on the following report from Smatch, fix the potential
> dereferencing freed memory check.
> 
>   tools/perf/util/annotate.c:1125
>   disasm_line__parse() error: dereferencing freed memory 'namep'
> 
> tools/perf/util/annotate.c
> 1100 static int disasm_line__parse(char *line, const char **namep, char **rawp)
> 1101 {
> 1102         char tmp, *name = ltrim(line);
> 
> [...]
> 
> 1114         *namep = strdup(name);
> 1115
> 1116         if (*namep == NULL)
> 1117                 goto out_free_name;
> 
> [...]
> 
> 1124 out_free_name:
> 1125         free((void *)namep);
>                           ^^^^^
> 1126         *namep = NULL;
>              ^^^^^^
> 1127         return -1;
> 1128 }
> 
> If strdup() fails to allocate memory space for *namep, we don't need to
> free memory with pointer 'namep', which is resident in data structure
> disasm_line::ins::name; and *namep is NULL pointer for this failure, so
> it's pointless to assign NULL to *namep again.

Applied, with this extra comment:


Committer note:

Freeing namep, which is the address of the first entry of the 'struct
ins' that is the first member of struct disasm_line would in fact free
that disasm_line instance, if it was allocated via malloc/calloc, which,
later, would a dereference of freed memory.

Re: [PATCH v1 05/11] perf trace: Smatch: Fix potential NULL pointer dereference

[Arnaldo Carvalho de Melo]

Em Tue, Jul 02, 2019 at 06:34:14PM +0800, Leo Yan escreveu:
> Based on the following report from Smatch, fix the potential
> NULL pointer dereference check.
> 
>   tools/perf/builtin-trace.c:1044
>   thread_trace__new() error: we previously assumed 'ttrace' could be
>   null (see line 1041).
> 
> tools/perf/builtin-trace.c
> 1037 static struct thread_trace *thread_trace__new(void)
> 1038 {
> 1039         struct thread_trace *ttrace =  zalloc(sizeof(struct thread_trace));
> 1040
> 1041         if (ttrace)
> 1042                 ttrace->files.max = -1;
> 1043
> 1044         ttrace->syscall_stats = intlist__new(NULL);
>              ^^^^^^^^
> 1045
> 1046         return ttrace;
> 1047 }
> 
> This patch directly returns NULL when fail to allocate memory for
> ttrace; this can avoid potential NULL pointer dereference.

I did it slightly different, to avoid having two return lines, and that
is what most other constructors (foo__new()) do in tools/perf/, kept
your changeset comment and patch authorship, thanks.

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index d0eb7224dd36..e3fc9062f136 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1038,10 +1038,10 @@ static struct thread_trace *thread_trace__new(void)
 {
 	struct thread_trace *ttrace =  zalloc(sizeof(struct thread_trace));
 
-	if (ttrace)
+	if (ttrace) {
 		ttrace->files.max = -1;
-
-	ttrace->syscall_stats = intlist__new(NULL);
+		ttrace->syscall_stats = intlist__new(NULL);
+	}
 
 	return ttrace;
 }

Re: [PATCH v1 08/11] perf session: Smatch: Fix potential NULL pointer dereference

[Arnaldo Carvalho de Melo]

Em Tue, Jul 02, 2019 at 06:34:17PM +0800, Leo Yan escreveu:
> Based on the following report from Smatch, fix the potential
> NULL pointer dereference check.
> 
>   tools/perf/util/session.c:1252
>   dump_read() error: we previously assumed 'evsel' could be null
>   (see line 1249)
> 
> tools/perf/util/session.c
> 1240 static void dump_read(struct perf_evsel *evsel, union perf_event *event)
> 1241 {
> 1242         struct read_event *read_event = &event->read;
> 1243         u64 read_format;
> 1244
> 1245         if (!dump_trace)
> 1246                 return;
> 1247
> 1248         printf(": %d %d %s %" PRIu64 "\n", event->read.pid, event->read.tid,
> 1249                evsel ? perf_evsel__name(evsel) : "FAIL",
> 1250                event->read.value);
> 1251
> 1252         read_format = evsel->attr.read_format;
>                            ^^^^^^^
> 
> 'evsel' could be NULL pointer, for this case this patch directly bails
> out without dumping read_event.

So this needs another hunk, adding it.

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 8e0e06d3edfc..f4591a1438b4 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -224,7 +224,7 @@ static int perf_event__repipe_sample(struct perf_tool *tool,
 				     struct perf_evsel *evsel,
 				     struct machine *machine)
 {
-	if (evsel->handler) {
+	if (evsel && evsel->handler) {
 		inject_handler f = evsel->handler;
 		return f(tool, event, sample, evsel, machine);
 	}
 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/util/session.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 54cf163347f7..2e61dd6a3574 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1249,6 +1249,9 @@ static void dump_read(struct perf_evsel *evsel, union perf_event *event)
>  	       evsel ? perf_evsel__name(evsel) : "FAIL",
>  	       event->read.value);
>  
> +	if (!evsel)
> +		return;
> +
>  	read_format = evsel->attr.read_format;
>  
>  	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> -- 
> 2.17.1

-- 

- Arnaldo

Re: [PATCH v1 00/11] perf: Fix errors detected by Smatch

[Leo Yan]

Hi Arnaldo,

On Wed, Jul 03, 2019 at 03:18:15PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jul 03, 2019 at 09:48:08AM +0800, Leo Yan escreveu:
> > On Tue, Jul 02, 2019 at 01:07:43PM +0200, Jiri Olsa wrote:
> > > On Tue, Jul 02, 2019 at 06:34:09PM +0800, Leo Yan wrote:
> > > > When I used static checker Smatch for perf building, the main target is
> > > > to check if there have any potential issues in Arm cs-etm code.  So
> > > > finally I get many reporting for errors/warnings.
> > > > 
> > > > I used below command for using static checker with perf building:
> > > > 
> > > >   # make VF=1 CORESIGHT=1 -C tools/perf/ \
> > > >     CHECK="/root/Work/smatch/smatch --full-path" \
> > > >     CC=/root/Work/smatch/cgcc | tee smatch_reports.txt
> > > > 
> > > > I reviewed the errors one by one, if I understood some of these errors
> > > > so changed the code as I can, this patch set is the working result; but
> > > > still leave some errors due to I don't know what's the best way to fix
> > > > it.  There also have many inconsistent indenting warnings.  So I firstly
> > > > send out this patch set and let's see what's the feedback from public
> > > > reviewing.
> > > > 
> > > > Leo Yan (11):
> > > >   perf report: Smatch: Fix potential NULL pointer dereference
> > > >   perf stat: Smatch: Fix use-after-freed pointer
> > > >   perf top: Smatch: Fix potential NULL pointer dereference
> > > >   perf annotate: Smatch: Fix dereferencing freed memory
> > > >   perf trace: Smatch: Fix potential NULL pointer dereference
> > > >   perf hists: Smatch: Fix potential NULL pointer dereference
> > > >   perf map: Smatch: Fix potential NULL pointer dereference
> > > >   perf session: Smatch: Fix potential NULL pointer dereference
> > > >   perf intel-bts: Smatch: Fix potential NULL pointer dereference
> > > >   perf intel-pt: Smatch: Fix potential NULL pointer dereference
> > > >   perf cs-etm: Smatch: Fix potential NULL pointer dereference
> > > 
> > > from quick look it all looks good to me, nice tool ;-)
> > > 
> > > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > 
> > Thanks for reviewing, Jiri.
> > 
> > @Arnaldo, Just want to check, will you firstly pick up 01~05, 07,
> > 08/11 patches if you think they are okay?  Or you want to wait me to
> > spin new patch set with all patches after I gather all comments?
> 
> I'm picking up the uncontrovertial, will push to my perf/core branch,
> continue from there, please.

Will do it.  Thank you much for amendment in these patches.

Leo.

[tip:perf/urgent] perf stat: Fix use-after-freed pointer detected by the smatch tool

[tip-bot for Leo Yan]

Commit-ID:  c74b05030edb3b52f4208d8415b8c933bc509a29
Gitweb:     https://git.kernel.org/tip/c74b05030edb3b52f4208d8415b8c933bc509a29
Author:     Leo Yan <leo.yan@linaro.org>
AuthorDate: Tue, 2 Jul 2019 18:34:11 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 9 Jul 2019 09:33:54 -0300

perf stat: Fix use-after-freed pointer detected by the smatch tool

Based on the following report from Smatch, fix the use-after-freed
pointer.

  tools/perf/builtin-stat.c:1353
  add_default_attributes() warn: passing freed memory 'str'.

The pointer 'str' has been freed but later it is still passed into the
function parse_events_print_error().  This patch fixes this
use-after-freed issue.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Alexios Zavras <alexios.zavras@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Saint-Etienne <eric.saint.etienne@oracle.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Song Liu <songliubraving@fb.com>
Cc: Suzuki Poulouse <suzuki.poulose@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Richter <tmricht@linux.ibm.com>
Link: http://lkml.kernel.org/r/20190702103420.27540-3-leo.yan@linaro.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index e5e19b461061..b81f7b197d24 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1349,8 +1349,8 @@ static int add_default_attributes(void)
 				fprintf(stderr,
 					"Cannot set up top down events %s: %d\n",
 					str, err);
-				free(str);
 				parse_events_print_error(&errinfo, str);
+				free(str);
 				return -1;
 			}
 		} else {

[tip:perf/urgent] perf top: Fix potential NULL pointer dereference detected by the smatch tool

[tip-bot for Leo Yan]

Commit-ID:  111442cfc8abdeaa7ec1407f07ef7b3e5f76654e
Gitweb:     https://git.kernel.org/tip/111442cfc8abdeaa7ec1407f07ef7b3e5f76654e
Author:     Leo Yan <leo.yan@linaro.org>
AuthorDate: Tue, 2 Jul 2019 18:34:12 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 9 Jul 2019 09:33:54 -0300

perf top: Fix potential NULL pointer dereference detected by the smatch tool

Based on the following report from Smatch, fix the potential NULL
pointer dereference check.

  tools/perf/builtin-top.c:109
  perf_top__parse_source() warn: variable dereferenced before check 'he'
  (see line 103)

  tools/perf/builtin-top.c:233
  perf_top__show_details() warn: variable dereferenced before check 'he'
  (see line 228)

  tools/perf/builtin-top.c
  101 static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
  102 {
  103         struct perf_evsel *evsel = hists_to_evsel(he->hists);
                                                        ^^^^
  104         struct symbol *sym;
  105         struct annotation *notes;
  106         struct map *map;
  107         int err = -1;
  108
  109         if (!he || !he->ms.sym)
  110                 return -1;

This patch moves the values assignment after validating pointer 'he'.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Alexios Zavras <alexios.zavras@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Eric Saint-Etienne <eric.saint.etienne@oracle.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Song Liu <songliubraving@fb.com>
Cc: Suzuki Poulouse <suzuki.poulose@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Richter <tmricht@linux.ibm.com>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20190702103420.27540-4-leo.yan@linaro.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 6d40a4ef58c5..b46b3c9f57a0 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -101,7 +101,7 @@ static void perf_top__resize(struct perf_top *top)
 
 static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
 {
-	struct perf_evsel *evsel = hists_to_evsel(he->hists);
+	struct perf_evsel *evsel;
 	struct symbol *sym;
 	struct annotation *notes;
 	struct map *map;
@@ -110,6 +110,8 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
 	if (!he || !he->ms.sym)
 		return -1;
 
+	evsel = hists_to_evsel(he->hists);
+
 	sym = he->ms.sym;
 	map = he->ms.map;
 
@@ -226,7 +228,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 static void perf_top__show_details(struct perf_top *top)
 {
 	struct hist_entry *he = top->sym_filter_entry;
-	struct perf_evsel *evsel = hists_to_evsel(he->hists);
+	struct perf_evsel *evsel;
 	struct annotation *notes;
 	struct symbol *symbol;
 	int more;
@@ -234,6 +236,8 @@ static void perf_top__show_details(struct perf_top *top)
 	if (!he)
 		return;
 
+	evsel = hists_to_evsel(he->hists);
+
 	symbol = he->ms.sym;
 	notes = symbol__annotation(symbol);
 

[tip:perf/urgent] perf annotate: Fix dereferencing freed memory found by the smatch tool

[tip-bot for Leo Yan]

Commit-ID:  600c787dbf6521d8d07ee717ab7606d5070103ea
Gitweb:     https://git.kernel.org/tip/600c787dbf6521d8d07ee717ab7606d5070103ea
Author:     Leo Yan <leo.yan@linaro.org>
AuthorDate: Tue, 2 Jul 2019 18:34:13 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 9 Jul 2019 09:33:55 -0300

perf annotate: Fix dereferencing freed memory found by the smatch tool

Based on the following report from Smatch, fix the potential
dereferencing freed memory check.

  tools/perf/util/annotate.c:1125
  disasm_line__parse() error: dereferencing freed memory 'namep'

  tools/perf/util/annotate.c
  1100 static int disasm_line__parse(char *line, const char **namep, char **rawp)
  1101 {
  1102         char tmp, *name = ltrim(line);

  [...]

  1114         *namep = strdup(name);
  1115
  1116         if (*namep == NULL)
  1117                 goto out_free_name;

  [...]

  1124 out_free_name:
  1125         free((void *)namep);
                            ^^^^^
  1126         *namep = NULL;
               ^^^^^^
  1127         return -1;
  1128 }

If strdup() fails to allocate memory space for *namep, we don't need to
free memory with pointer 'namep', which is resident in data structure
disasm_line::ins::name; and *namep is NULL pointer for this failure, so
it's pointless to assign NULL to *namep again.

Committer note:

Freeing namep, which is the address of the first entry of the 'struct
ins' that is the first member of struct disasm_line would in fact free
that disasm_line instance, if it was allocated via malloc/calloc, which,
later, would a dereference of freed memory.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Alexios Zavras <alexios.zavras@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Eric Saint-Etienne <eric.saint.etienne@oracle.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Song Liu <songliubraving@fb.com>
Cc: Suzuki Poulouse <suzuki.poulose@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Richter <tmricht@linux.ibm.com>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20190702103420.27540-5-leo.yan@linaro.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/annotate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index ec7aaf31c2b2..944a6507a5e3 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1119,16 +1119,14 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
 	*namep = strdup(name);
 
 	if (*namep == NULL)
-		goto out_free_name;
+		goto out;
 
 	(*rawp)[0] = tmp;
 	*rawp = skip_spaces(*rawp);
 
 	return 0;
 
-out_free_name:
-	free((void *)namep);
-	*namep = NULL;
+out:
 	return -1;
 }
 

[tip:perf/urgent] perf trace: Fix potential NULL pointer dereference found by the smatch tool

[tip-bot for Leo Yan]

Commit-ID:  7a6d49dc8cad8fa1f3d63994102af8f9ae9c859f
Gitweb:     https://git.kernel.org/tip/7a6d49dc8cad8fa1f3d63994102af8f9ae9c859f
Author:     Leo Yan <leo.yan@linaro.org>
AuthorDate: Tue, 2 Jul 2019 18:34:14 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 9 Jul 2019 09:33:55 -0300

perf trace: Fix potential NULL pointer dereference found by the smatch tool

Based on the following report from Smatch, fix the potential NULL
pointer dereference check.

  tools/perf/builtin-trace.c:1044
  thread_trace__new() error: we previously assumed 'ttrace' could be
  null (see line 1041).

  tools/perf/builtin-trace.c
  1037 static struct thread_trace *thread_trace__new(void)
  1038 {
  1039         struct thread_trace *ttrace =  zalloc(sizeof(struct thread_trace));
  1040
  1041         if (ttrace)
  1042                 ttrace->files.max = -1;
  1043
  1044         ttrace->syscall_stats = intlist__new(NULL);
               ^^^^^^^^
  1045
  1046         return ttrace;
  1047 }

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Alexios Zavras <alexios.zavras@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Eric Saint-Etienne <eric.saint.etienne@oracle.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Song Liu <songliubraving@fb.com>
Cc: Suzuki Poulouse <suzuki.poulose@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Richter <tmricht@linux.ibm.com>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20190702103420.27540-6-leo.yan@linaro.org
[ Just made it look like other tools/perf constructors, same end result ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-trace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index d0eb7224dd36..e3fc9062f136 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1038,10 +1038,10 @@ static struct thread_trace *thread_trace__new(void)
 {
 	struct thread_trace *ttrace =  zalloc(sizeof(struct thread_trace));
 
-	if (ttrace)
+	if (ttrace) {
 		ttrace->files.max = -1;
-
-	ttrace->syscall_stats = intlist__new(NULL);
+		ttrace->syscall_stats = intlist__new(NULL);
+	}
 
 	return ttrace;
 }

[tip:perf/urgent] perf map: Fix potential NULL pointer dereference found by smatch tool

[tip-bot for Leo Yan]

Commit-ID:  363bbaef63ffebcc745239fe80a953ebb5ac9ec9
Gitweb:     https://git.kernel.org/tip/363bbaef63ffebcc745239fe80a953ebb5ac9ec9
Author:     Leo Yan <leo.yan@linaro.org>
AuthorDate: Tue, 2 Jul 2019 18:34:16 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 9 Jul 2019 09:33:55 -0300

perf map: Fix potential NULL pointer dereference found by smatch tool

Based on the following report from Smatch, fix the potential NULL
pointer dereference check.

  tools/perf/util/map.c:479
  map__fprintf_srccode() error: we previously assumed 'state' could be
  null (see line 466)

  tools/perf/util/map.c
  465         /* Avoid redundant printing */
  466         if (state &&
  467             state->srcfile &&
  468             !strcmp(state->srcfile, srcfile) &&
  469             state->line == line) {
  470                 free(srcfile);
  471                 return 0;
  472         }
  473
  474         srccode = find_sourceline(srcfile, line, &len);
  475         if (!srccode)
  476                 goto out_free_line;
  477
  478         ret = fprintf(fp, "|%-8d %.*s", line, len, srccode);
  479         state->srcfile = srcfile;
              ^^^^^^^
  480         state->line = line;
              ^^^^^^^

This patch validates 'state' pointer before access its elements.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Alexios Zavras <alexios.zavras@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Eric Saint-Etienne <eric.saint.etienne@oracle.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Song Liu <songliubraving@fb.com>
Cc: Suzuki Poulouse <suzuki.poulose@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Richter <tmricht@linux.ibm.com>
Cc: linux-arm-kernel@lists.infradead.org
Fixes: dd2e18e9ac20 ("perf tools: Support 'srccode' output")
Link: http://lkml.kernel.org/r/20190702103420.27540-8-leo.yan@linaro.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/map.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 6fce983c6115..5f87975d2562 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -476,8 +476,11 @@ int map__fprintf_srccode(struct map *map, u64 addr,
 		goto out_free_line;
 
 	ret = fprintf(fp, "|%-8d %.*s", line, len, srccode);
-	state->srcfile = srcfile;
-	state->line = line;
+
+	if (state) {
+		state->srcfile = srcfile;
+		state->line = line;
+	}
 	return ret;
 
 out_free_line:

[tip:perf/urgent] perf session: Fix potential NULL pointer dereference found by the smatch tool

[tip-bot for Leo Yan]

Commit-ID:  f3c8d90757724982e5f07cd77d315eb64ca145ac
Gitweb:     https://git.kernel.org/tip/f3c8d90757724982e5f07cd77d315eb64ca145ac
Author:     Leo Yan <leo.yan@linaro.org>
AuthorDate: Tue, 2 Jul 2019 18:34:17 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 9 Jul 2019 09:33:55 -0300

perf session: Fix potential NULL pointer dereference found by the smatch tool

Based on the following report from Smatch, fix the potential
NULL pointer dereference check.

  tools/perf/util/session.c:1252
  dump_read() error: we previously assumed 'evsel' could be null
  (see line 1249)

  tools/perf/util/session.c
  1240 static void dump_read(struct perf_evsel *evsel, union perf_event *event)
  1241 {
  1242         struct read_event *read_event = &event->read;
  1243         u64 read_format;
  1244
  1245         if (!dump_trace)
  1246                 return;
  1247
  1248         printf(": %d %d %s %" PRIu64 "\n", event->read.pid, event->read.tid,
  1249                evsel ? perf_evsel__name(evsel) : "FAIL",
  1250                event->read.value);
  1251
  1252         read_format = evsel->attr.read_format;
                             ^^^^^^^

'evsel' could be NULL pointer, for this case this patch directly bails
out without dumping read_event.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Alexios Zavras <alexios.zavras@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Eric Saint-Etienne <eric.saint.etienne@oracle.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Song Liu <songliubraving@fb.com>
Cc: Suzuki Poulouse <suzuki.poulose@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Richter <tmricht@linux.ibm.com>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20190702103420.27540-9-leo.yan@linaro.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 54cf163347f7..2e61dd6a3574 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1249,6 +1249,9 @@ static void dump_read(struct perf_evsel *evsel, union perf_event *event)
 	       evsel ? perf_evsel__name(evsel) : "FAIL",
 	       event->read.value);
 
+	if (!evsel)
+		return;
+
 	read_format = evsel->attr.read_format;
 
 	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)