Re: [PATCH] perf diff: Don't crash on freeing errno-session
Em Tue, Mar 02, 2021 at 01:47:55PM +0900, Namhyung Kim escreveu: > Hello, > > On Tue, Mar 2, 2021 at 11:35 AM Dmitry Safonov wrote: > > > > __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 0x5569a62b5955 in auxtrace__free (session=0x) > > at util/auxtrace.c:2681 > > 1 0x5569a626b37d in perf_session__delete (session=0x) > > at util/session.c:295 > > 2 perf_session__delete (session=0x) at util/session.c:291 > > 3 0x5569a618008a in __cmd_diff () at builtin-diff.c:1239 > > 4 cmd_diff (argc=, argv=) 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. > > > > Cc: Alexander Shishkin > > Cc: Arnaldo Carvalho de Melo > > Cc: Ingo Molnar > > Cc: Jiri Olsa > > Cc: Mark Rutland > > Cc: Namhyung Kim > > Cc: Peter Zijlstra > > Signed-off-by: Dmitry Safonov > > Acked-by: Namhyung Kim Thanks, tested, added a complete set of steps for a problem to be reproduced and applied. - Arnaldo 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] 0x8825de16 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 Acked-by: Namhyung Kim Tested-by: Arnaldo Carvalho de Melo
Re: [PATCH] perf diff: Don't crash on freeing errno-session
Hello,
On Tue, Mar 2, 2021 at 11:35 AM Dmitry Safonov wrote:
>
> __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 0x5569a62b5955 in auxtrace__free (session=0x)
> at util/auxtrace.c:2681
> 1 0x5569a626b37d in perf_session__delete (session=0x)
> at util/session.c:295
> 2 perf_session__delete (session=0x) at util/session.c:291
> 3 0x5569a618008a in __cmd_diff () at builtin-diff.c:1239
> 4 cmd_diff (argc=, argv=) 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.
>
> Cc: Alexander Shishkin
> Cc: Arnaldo Carvalho de Melo
> Cc: Ingo Molnar
> Cc: Jiri Olsa
> Cc: Mark Rutland
> Cc: Namhyung Kim
> Cc: Peter Zijlstra
> Signed-off-by: Dmitry Safonov
Acked-by: Namhyung Kim
Thanks,
Namhyung
> ---
> tools/perf/builtin-diff.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index cefc71506409..b0c57e55052d 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -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);
> }
>
> --
> 2.30.1
>
[PATCH] perf diff: Don't crash on freeing errno-session
__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 0x5569a62b5955 in auxtrace__free (session=0x)
at util/auxtrace.c:2681
1 0x5569a626b37d in perf_session__delete (session=0x)
at util/session.c:295
2 perf_session__delete (session=0x) at util/session.c:291
3 0x5569a618008a in __cmd_diff () at builtin-diff.c:1239
4 cmd_diff (argc=, argv=) 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.
Cc: Alexander Shishkin
Cc: Arnaldo Carvalho de Melo
Cc: Ingo Molnar
Cc: Jiri Olsa
Cc: Mark Rutland
Cc: Namhyung Kim
Cc: Peter Zijlstra
Signed-off-by: Dmitry Safonov
---
tools/perf/builtin-diff.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index cefc71506409..b0c57e55052d 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -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);
}
--
2.30.1
