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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
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
>
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 <[email protected]>
> ---
> 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;
> }
>
>
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 <[email protected]>
> ---
> 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 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 <[email protected]>
> > ---
> > 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;
> > }
> >
> >
>
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 <[email protected]>
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
> >
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.
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
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 <[email protected]>
> > ---
> > 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
> >
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 <[email protected]>
> > > ---
> > > 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;
> > > }
> > >
> > >
> >
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 <[email protected]>
> > > > ---
> > > > 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;
> > > > }
> > > >
> > > >
> > >
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>
> 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
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
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;
}
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.
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> >
> > 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.
Commit-ID: 111442cfc8abdeaa7ec1407f07ef7b3e5f76654e
Gitweb: https://git.kernel.org/tip/111442cfc8abdeaa7ec1407f07ef7b3e5f76654e
Author: Leo Yan <[email protected]>
AuthorDate: Tue, 2 Jul 2019 18:34:12 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: Alexios Zavras <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Changbin Du <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Eric Saint-Etienne <[email protected]>
Cc: Jin Yao <[email protected]>
Cc: Konstantin Khlebnikov <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Suzuki Poulouse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Thomas Richter <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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);
Commit-ID: c74b05030edb3b52f4208d8415b8c933bc509a29
Gitweb: https://git.kernel.org/tip/c74b05030edb3b52f4208d8415b8c933bc509a29
Author: Leo Yan <[email protected]>
AuthorDate: Tue, 2 Jul 2019 18:34:11 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: Alexios Zavras <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Changbin Du <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Eric Saint-Etienne <[email protected]>
Cc: Jin Yao <[email protected]>
Cc: Konstantin Khlebnikov <[email protected]>
Cc: [email protected]
Cc: Mathieu Poirier <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Suzuki Poulouse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Thomas Richter <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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 {
Commit-ID: 600c787dbf6521d8d07ee717ab7606d5070103ea
Gitweb: https://git.kernel.org/tip/600c787dbf6521d8d07ee717ab7606d5070103ea
Author: Leo Yan <[email protected]>
AuthorDate: Tue, 2 Jul 2019 18:34:13 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: Alexios Zavras <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Changbin Du <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Eric Saint-Etienne <[email protected]>
Cc: Jin Yao <[email protected]>
Cc: Konstantin Khlebnikov <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Suzuki Poulouse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Thomas Richter <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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;
}
Commit-ID: 363bbaef63ffebcc745239fe80a953ebb5ac9ec9
Gitweb: https://git.kernel.org/tip/363bbaef63ffebcc745239fe80a953ebb5ac9ec9
Author: Leo Yan <[email protected]>
AuthorDate: Tue, 2 Jul 2019 18:34:16 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: Alexios Zavras <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Changbin Du <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Eric Saint-Etienne <[email protected]>
Cc: Jin Yao <[email protected]>
Cc: Konstantin Khlebnikov <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Suzuki Poulouse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Thomas Richter <[email protected]>
Cc: [email protected]
Fixes: dd2e18e9ac20 ("perf tools: Support 'srccode' output")
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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:
Commit-ID: 7a6d49dc8cad8fa1f3d63994102af8f9ae9c859f
Gitweb: https://git.kernel.org/tip/7a6d49dc8cad8fa1f3d63994102af8f9ae9c859f
Author: Leo Yan <[email protected]>
AuthorDate: Tue, 2 Jul 2019 18:34:14 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: Alexios Zavras <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Changbin Du <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Eric Saint-Etienne <[email protected]>
Cc: Jin Yao <[email protected]>
Cc: Konstantin Khlebnikov <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Suzuki Poulouse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Thomas Richter <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
[ Just made it look like other tools/perf constructors, same end result ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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;
}
Commit-ID: f3c8d90757724982e5f07cd77d315eb64ca145ac
Gitweb: https://git.kernel.org/tip/f3c8d90757724982e5f07cd77d315eb64ca145ac
Author: Leo Yan <[email protected]>
AuthorDate: Tue, 2 Jul 2019 18:34:17 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: Alexios Zavras <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Changbin Du <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Eric Saint-Etienne <[email protected]>
Cc: Jin Yao <[email protected]>
Cc: Konstantin Khlebnikov <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Suzuki Poulouse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Thomas Richter <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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)