Re: [PATCH] bpf: selftests: global_funcs: check err_str before strstr

From: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
To: Yonghong Song <yhs@fb.com>
Cc: bpf <bpf@vger.kernel.org>, Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <ast@kernel.org>,
	Song Liu <songliubraving@fb.com>
Subject: Re: [PATCH] bpf: selftests: global_funcs: check err_str before strstr
Date: Wed, 19 Aug 2020 23:46:37 +0300	[thread overview]
Message-ID: <CANoWswnJ0fh_uMzwuLDyZsyWcNyiwMnx4-Unp=q22ppVe8f0-w@mail.gmail.com> (raw)
In-Reply-To: <1518db68-6699-b261-7b6e-2d71c3d9566c@fb.com>

On Wed, Aug 19, 2020 at 5:57 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 8/19/20 12:05 AM, Yauheni Kaliuta wrote:
> > On Wed, Aug 19, 2020 at 8:19 AM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> On 8/18/20 7:34 PM, Yauheni Kaliuta wrote:
> >>> The error path in libbpf.c:load_program() has calls to pr_warn()
> >>> which ends up for global_funcs tests to
> >>> test_global_funcs.c:libbpf_debug_print().
> >>>
> >>> For the tests with no struct test_def::err_str initialized with a
> >>> string, it causes call of strstr() with NULL as the second argument
> >>> and it segfaults.
> >>>
> >>> Fix it by calling strstr() only for non-NULL err_str.
> >>>
> >>> The patch does not fix the test itself.
> >>
> >> So this happens in older kernel, right? Could you clarify more
> >> in which kernel and what environment? It probably no need to
> >> fix the issue for really old kernel but some clarification
> >> will be good.
> >
> > I'll test it with the very recent kernel on that architecture soon,
> > for sure. But it's not related to the patch.
>
> The above "The patch does not fix the test itself" a little bit vague.
> You can say that "The test may fail in old kernels where <why it fails
> ...> and this patch is to fix the segfault rather the test failure.".
> This way people can easily understand why and the purpose of this patch.

Ok, I'll remove it completely (as I mentioned in the follow-up email,
the test still fails for me for the Linus' HEAD).

>
> >
> >>
> >>>
> >>> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
>
> Ok, ack with the above nit and one nit below.
>     Acked-by: Yonghong Song <yhs@fb.com>
> I guess it is better to send a v2 carrying my ack.
>
> >>> ---
> >>>    tools/testing/selftests/bpf/prog_tests/test_global_funcs.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> >>> index 25b068591e9a..6ad14c5465eb 100644
> >>> --- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> >>> +++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> >>> @@ -19,7 +19,7 @@ static int libbpf_debug_print(enum libbpf_print_level level,
> >>>        log_buf = va_arg(args, char *);
> >>>        if (!log_buf)
> >>>                goto out;
> >>> -     if (strstr(log_buf, err_str) == 0)
> >>> +     if ((err_str != NULL) && (strstr(log_buf, err_str) == 0))
> >>
> >> Looks good but the code can be simplified as
> >>          if (err_str && strstr(log_buf, err_str) == 0)
> >>                  found = true;
> >
> > Yes, but I prefer to use NULL explicitly when I deal with pointers. It
> > demonstrates intention better. You also can simplify strstr() == 0
> > with !. Actually, strstr() returns char *, so comparation to 0
> > (totally legal by standard) breaks my feelings too :).
>
> comparison with NULL is okay. You can just do
>     (err_str != NULL && strstr(log_buf, err_str) == 0)
> there is no need for extra parenthesis.

Ah, ok. Inconsistency with the strstr check bothers me, but it would
be unrelated change.

Thank you for review!

>
> >
> > If you insist, I'll send v2 of course.
> >
> >>>                found = true;
> >>>    out:
> >>>        printf(format, log_buf);
> >>>
> >>
> >
> >
>

-- 
WBR, Yauheni