Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jiri Olsa <olsajiri@gmail.com>
Date:   Tue, 27 Jun 2023 10:35:28 +0200
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Ingo Molnar <mingo@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Ian Rogers <irogers@google.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        linux-perf-users@vger.kernel.org
Subject: Re: [PATCH] perf tools: Add missing else to cmd_daemon subcommand
 condition
Message-ID: <ZJqfUL1gnkTbUx5n@krava>
References: <20230626201606.2514679-1-jolsa@kernel.org>
 <CAM9d7chMGzEJ3GKyxXD-RwUpjje7u3Cp810TRD+ZGgCT+mFrEQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAM9d7chMGzEJ3GKyxXD-RwUpjje7u3Cp810TRD+ZGgCT+mFrEQ@mail.gmail.com>
Precedence: bulk

On Mon, Jun 26, 2023 at 01:58:48PM -0700, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Mon, Jun 26, 2023 at 1:16 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Namhyung reported segfault in perf daemon start command.
> >
> > It's caused by extra check on argv[0] which is set to NULL by previous
> > __cmd_start call. Adding missing else to skip the extra check.
> >
> > Fixes: 92294b906e6c ("perf daemon: Dynamically allocate path to perf")
> > Reported-by: Namhyung Kim <namhyung@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Thanks for the fix.  Now it runs ok.
> 
> Before:
>   $ sudo ./perf test -v daemon
>    85: daemon operations                                               :
>   --- start ---
>   test child forked, pid 82420
>   test daemon list
>   ./tests/shell/daemon.sh: line 133: 82426 Segmentation fault
> perf daemon start --config ${config}
>   test daemon reconfig
>   ./tests/shell/daemon.sh: line 133: 82520 Segmentation fault
> perf daemon start --config ${config}
>   test daemon stop
>   ./tests/shell/daemon.sh: line 133: 82636 Segmentation fault
> perf daemon start --config ${config}
>   test daemon signal
>   ./tests/shell/daemon.sh: line 133: 82674 Segmentation fault
> perf daemon start --config ${config}
>   signal 12 sent to session 'test [82676]'
>   signal 12 sent to session 'test [82676]'
>   test daemon ping
>   ./tests/shell/daemon.sh: line 133: 82702 Segmentation fault
> perf daemon start --config ${config}
>   test daemon lock
>   ./tests/shell/daemon.sh: line 133: 82734 Segmentation fault
> perf daemon start --config ${config}
>   test child finished with 0
>   ---- end ----
>   daemon operations: Ok
> 
> Maybe we need to investigate more why it was ok..
> But at least I don't see segfaults anymore

yea, for some reason parse_options would put NULL into argv[0]

I'll try to check what changed, in any case the fix makes the
condition alligned with the other legs and fixes the segfault

jirka

> 
> 
> After:
>    85: daemon operations                                               :
>   --- start ---
>   test child forked, pid 80752
>   test daemon list
>   test daemon reconfig
>   test daemon stop
>   test daemon signal
>   signal 12 sent to session 'test [81022]'
>   signal 12 sent to session 'test [81022]'
>   test daemon ping
>   test daemon lock
>   test child finished with 0
>   ---- end ----
>   daemon operations: Ok
> 
> 
> Tested-by: Namhyung Kim <namhyung@kernel.org>
> 
> Thanks,
> Namhyung
> 
> 
> 
> > ---
> >  tools/perf/builtin-daemon.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> > index f5674d824a40..83954af36753 100644
> > --- a/tools/perf/builtin-daemon.c
> > +++ b/tools/perf/builtin-daemon.c
> > @@ -1524,7 +1524,7 @@ int cmd_daemon(int argc, const char **argv)
> >         if (argc) {
> >                 if (!strcmp(argv[0], "start"))
> >                         ret = __cmd_start(&__daemon, daemon_options, argc, argv);
> > -               if (!strcmp(argv[0], "signal"))
> > +               else if (!strcmp(argv[0], "signal"))
> >                         ret = __cmd_signal(&__daemon, daemon_options, argc, argv);
> >                 else if (!strcmp(argv[0], "stop"))
> >                         ret = __cmd_stop(&__daemon, daemon_options, argc, argv);
> > --
> > 2.41.0
> >