Re: [PATCH 3/3] rtla: Parse cmdline using libsubcmd

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Segmentation fault when parsing abbreviated help options (`--he`, `--hel`) due to `RTLA_OPT_HELP` macro definition.
- [High] Options `-u/--user-threads` and `-n/--nano` incorrectly swallow subsequent arguments or fail when placed at the end of the command line.
--

commit 7b68f92828ddb67d26e43dca96fdb16d529c279d
Author: Tomas Glozar <tglozar@redhat.com>

rtla: Parse cmdline using libsubcmd

This commit moves RTLA command line parsing from raw getopt_long() to
libsubcmd's parse_options(). It defines reusable macros for common and
tool-specific options.

> diff --git a/tools/tracing/rtla/src/cli.c b/tools/tracing/rtla/src/cli.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/tracing/rtla/src/cli.c
[ ... ]
> +#define RTLA_OPT_CGROUP OPT_CALLBACK_OPTARG('C', "cgroup", &params->common, \
> +	"[cgroup_name]", NULL, \
> +	"set cgroup, no argument means rtla's cgroup will be inherited", \
> +	opt_cgroup_cb)
> +
> +#define RTLA_OPT_USER_THREADS OPT_CALLBACK('u', "user-threads", params, NULL, \
> +	"use rtla user-space threads instead of kernel-space timerlat threads", \
> +	opt_user_threads_cb)

Can this swallow subsequent command-line arguments or fail at the end of the
argument list?

Since OPT_CALLBACK is used here without PARSE_OPT_NOARG, libsubcmd's
get_arg() will strictly expect a value. If someone runs 
"rtla timerlat top -u -p 100", the "-p" would be consumed as the argument to
"-u". If "-u" is placed at the end of the command line, it fails expecting
a value.

Could OPT_CALLBACK_NOOPT or OPT_BOOLEAN be used here instead?

[ ... ]
> +#define RTLA_OPT_DEBUG OPT_BOOLEAN('D', "debug", &config_debug, \
> +	"print debug info")
> +
> +#define RTLA_OPT_HELP OPT_BOOLEAN('h', "help", (bool *)NULL, \
> +	"show help")

Can this cause a segmentation fault if an abbreviated help option is provided?

While exact matches for "-h" and "--help" are intercepted early by libsubcmd,
partial matches like "--he", "--hel", or negated "--no-help" bypass the early
check and perform standard prefix matching. This leads to get_value() being
called for the boolean option, which attempts to dereference the NULL value
pointer.

[ ... ]
> +#define TIMERLAT_OPT_STACK OPT_CALLBACK('s', "stack", &params->print_stack, "us", \
> +	"save the stack trace at the IRQ if a thread latency is higher than the argument in us", \
> +	opt_llong_callback)
> +
> +#define TIMERLAT_OPT_NANO OPT_CALLBACK('n', "nano", params, NULL, \
> +	"display data in nanoseconds", \
> +	opt_nano_cb)

Does this option also unintentionally consume the following argument?

Similar to RTLA_OPT_USER_THREADS, using OPT_CALLBACK without a flag preventing
argument consumption causes libsubcmd to require a value. Could
OPT_CALLBACK_NOOPT be used to ensure the flag behaves correctly as a toggle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260320150651.51057-1-tglozar@redhat.com?part=3