netdev - Re: [PATCH bpf-next v1 2/8] selftests/bpf: Fix arg parsing in veristat, test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bzbb3hPzKN2-wEg24JNMaJsUDiwpx9=xjvOK7TnAaSk4wQ@mail.gmail.com>
Date: Thu, 25 Jul 2024 13:09:24 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Tony Ambardar <tony.ambardar@...il.com>
Cc: bpf@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	netdev@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>, 
	Björn Töpel <bjorn@...nel.org>, 
	Magnus Karlsson <magnus.karlsson@...el.com>, 
	Maciej Fijalkowski <maciej.fijalkowski@...el.com>, Jonathan Lemon <jonathan.lemon@...il.com>, 
	"David S. Miller" <davem@...emloft.net>, Yan Zhai <yan@...udflare.com>
Subject: Re: [PATCH bpf-next v1 2/8] selftests/bpf: Fix arg parsing in
 veristat, test_progs

On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@...il.com> wrote:
>
> From: Tony Ambardar <tony.ambardar@...il.com>
>
> Current code parses arguments with strtok_r() using a construct like
>
>     char *state = NULL;
>     while ((next = strtok_r(state ? NULL : input, ",", &state))) {
>         ...
>     }
>
> where logic assumes the 'state' var can distinguish between first and
> subsequent strtok_r() calls, and adjusts parameters accordingly. However,
> 'state' is strictly internal context for strtok_r() and no such assumptions
> are supported in the man page. Moreover, the exact behaviour of 'state'
> depends on the libc implementation, making the above code fragile.
>
> Indeed, invoking "./test_progs -t <test_name>" on mips64el/musl will hang,
> with the above code in an infinite loop.
>
> Similarly, we see strange behaviour running 'veristat' on mips64el/musl:
>
>     $ ./veristat -e file,prog,verdict,insns -C two-ok add-failure
>     Can't specify more than 9 stats
>
> Rewrite code using a 'for' loop without logic dependent on var 'state', the
> same approach already used in cgroup_helpers.c.
>
> Fixes: 61ddff373ffa ("selftests/bpf: Improve by-name subtest selection logic in prog_tests")
> Fixes: 394169b079b5 ("selftests/bpf: add comparison mode to veristat")
> Fixes: c8bc5e050976 ("selftests/bpf: Add veristat tool for mass-verifying BPF object files")
> Signed-off-by: Tony Ambardar <tony.ambardar@...il.com>
> ---
>  tools/testing/selftests/bpf/testing_helpers.c | 2 +-
>  tools/testing/selftests/bpf/veristat.c        | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index ac7c66f4fc7b..2a73b72feb18 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -227,7 +227,7 @@ int parse_test_list(const char *s,
>         if (!input)
>                 return -ENOMEM;
>
> -       while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
> +       for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) {

oh, this is so long and verbose, let's just add a counter and use that
to determine whether to pass NULL or input, ok?

>                 err = insert_test(set, test_spec, is_glob_pattern);
>                 if (err)
>                         break;
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 11ec1190d582..6808679827ac 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -791,7 +791,7 @@ static int parse_stats(const char *stats_str, struct stat_specs *specs)
>         if (!input)
>                 return -ENOMEM;
>
> -       while ((next = strtok_r(state ? NULL : input, ",", &state))) {
> +       for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) {

ditto, let's not duplicate strtok_r() calls

>                 err = parse_stat(next, specs);
>                 if (err) {
>                         free(input);
> @@ -1513,7 +1513,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs,
>                         *stat_cntp += 1;
>                 }
>
> -               while ((next = strtok_r(state ? NULL : input, ",\n", &state))) {
> +               for (next = strtok_r(input, ",\n", &state); next; next = strtok_r(NULL, ",\n", &state)) {
>                         if (header) {
>                                 /* for the first line, set up spec stats */
>                                 err = parse_stat(next, specs);
> --
> 2.34.1
>