1
0
Fork 0

perf diff: Don't crash on freeing errno-session on the error path

__cmd_diff() sets result of perf_session__new() to d->session.

In case of failure, it's errno and perf-diff may crash with:

  failed to open perf.data: Permission denied
  Failed to open perf.data
  Segmentation fault (core dumped)

From the coredump:

0  0x00005569a62b5955 in auxtrace__free (session=0xffffffffffffffff)
    at util/auxtrace.c:2681
1  0x00005569a626b37d in perf_session__delete (session=0xffffffffffffffff)
    at util/session.c:295
2  perf_session__delete (session=0xffffffffffffffff) at util/session.c:291
3  0x00005569a618008a in __cmd_diff () at builtin-diff.c:1239
4  cmd_diff (argc=<optimized out>, argv=<optimized out>) at builtin-diff.c:2011
[..]

Funny enough, it won't always crash. For me it crashes only if failed
file is second in cmd-line: the reason is that cmd_diff() check files for
branch-stacks [in check_file_brstack()] and if the first file doesn't
have brstacks, it doesn't proceed to try open other files from cmd-line.

Check d->session before calling perf_session__delete().

Another solution would be assigning to temporary variable, checking it,
but I find it easier to follow with IS_ERR() check in the same function.
After some time it's still obvious why the check is needed, and with
temp variable it's possible to make the same mistake.

Committer testing:

  $ perf record sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.001 MB perf.data (8 samples) ]
  $ perf diff
  failed to open perf.data.old: No such file or directory
  Failed to open perf.data.old
  $ perf record sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.001 MB perf.data (8 samples) ]
  $ perf diff
  # Event 'cycles:u'
  #
  # Baseline  Delta Abs  Shared Object     Symbol
  # ........  .........  ................  ..........................
  #
       0.92%    +87.66%  [unknown]         [k] 0xffffffff8825de16
      11.39%     +0.04%  ld-2.32.so        [.] __GI___tunables_init
      87.70%             ld-2.32.so        [.] _dl_check_map_versions
  $ sudo chown root:root perf.data
  [sudo] password for acme:
  $ perf diff
  failed to open perf.data: Permission denied
  Failed to open perf.data
  Segmentation fault (core dumped)
  $

After the patch:

  $ perf diff
  failed to open perf.data: Permission denied
  Failed to open perf.data
  $

Signed-off-by: Dmitry Safonov <dima@arista.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: dmitry safonov <dima@arista.com>
Link: http://lore.kernel.org/lkml/20210302023533.1572231-1-dima@arista.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
master
Dmitry Safonov 2021-03-02 02:35:33 +00:00 committed by Arnaldo Carvalho de Melo
parent 2b1919ec83
commit ffc52b7ae5
1 changed files with 2 additions and 1 deletions

View File

@ -1236,7 +1236,8 @@ static int __cmd_diff(void)
out_delete:
data__for_each_file(i, d) {
perf_session__delete(d->session);
if (!IS_ERR(d->session))
perf_session__delete(d->session);
data__free(d);
}