Re: [PATCH v5 bpf-next 4/4] selftest/bpf: Extend the bpf_snprintf() test for "%c".

[Kuniyuki Iwashima <kuniyu@amazon.co.jp>]

From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Fri, 13 Aug 2021 16:30:29 -0700
> On Fri, Aug 13, 2021 at 4:28 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Aug 12, 2021 at 9:47 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> > >
> > > This patch adds a "positive" pattern for "%c", which intentionally uses a
> > > __u32 value (0x64636261, "dbca") to print a single character "a".  If the
> > > implementation went wrong, other 3 bytes might show up as the part of the
> > > latter "%+05s".
> > >
> > > Also, this patch adds two "negative" patterns for wide character.
> > >
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > ---
> > >  tools/testing/selftests/bpf/prog_tests/snprintf.c | 4 +++-
> > >  tools/testing/selftests/bpf/progs/test_snprintf.c | 7 ++++---
> > >  2 files changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c
> > > index dffbcaa1ec98..f77d7def7fed 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/snprintf.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c
> > > @@ -19,7 +19,7 @@
> > >  #define EXP_ADDR_OUT "0000000000000000 ffff00000add4e55 "
> > >  #define EXP_ADDR_RET sizeof(EXP_ADDR_OUT "unknownhashedptr")
> > >
> > > -#define EXP_STR_OUT  "str1 longstr"
> > > +#define EXP_STR_OUT  "str1         a longstr"
> > >  #define EXP_STR_RET  sizeof(EXP_STR_OUT)
> > >
> > >  #define EXP_OVER_OUT "%over"
> > > @@ -114,6 +114,8 @@ void test_snprintf_negative(void)
> > >         ASSERT_ERR(load_single_snprintf("%"), "invalid specifier 3");
> > >         ASSERT_ERR(load_single_snprintf("%12345678"), "invalid specifier 4");
> > >         ASSERT_ERR(load_single_snprintf("%--------"), "invalid specifier 5");
> > > +       ASSERT_ERR(load_single_snprintf("%lc"), "invalid specifier 6");
> > > +       ASSERT_ERR(load_single_snprintf("%llc"), "invalid specifier 7");
> > >         ASSERT_ERR(load_single_snprintf("\x80"), "non ascii character");
> > >         ASSERT_ERR(load_single_snprintf("\x1"), "non printable character");
> > >  }
> > > diff --git a/tools/testing/selftests/bpf/progs/test_snprintf.c b/tools/testing/selftests/bpf/progs/test_snprintf.c
> > > index e2ad26150f9b..afc2c583125b 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_snprintf.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_snprintf.c
> > > @@ -40,6 +40,7 @@ int handler(const void *ctx)
> > >         /* Convenient values to pretty-print */
> > >         const __u8 ex_ipv4[] = {127, 0, 0, 1};
> > >         const __u8 ex_ipv6[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1};
> > > +       const __u32 chr1 = 0x64636261; /* dcba */
> > >         static const char str1[] = "str1";
> > >         static const char longstr[] = "longstr";
> > >
> > > @@ -59,9 +60,9 @@ int handler(const void *ctx)
> > >         /* Kernel pointers */
> > >         addr_ret = BPF_SNPRINTF(addr_out, sizeof(addr_out), "%pK %px %p",
> > >                                 0, 0xFFFF00000ADD4E55, 0xFFFF00000ADD4E55);
> > > -       /* Strings embedding */
> > > -       str_ret  = BPF_SNPRINTF(str_out, sizeof(str_out), "%s %+05s",
> > > -                               str1, longstr);
> > > +       /* Strings and single-byte character embedding */
> > > +       str_ret  = BPF_SNPRINTF(str_out, sizeof(str_out), "%s % 9c %+05s",
> > > +                               str1, chr1, longstr);
> 
> Can you also add tests for %+2c, %-3c, %04c, %0c? Think outside the box ;)

Sure.


> > Why this hackery with __u32? You are making an endianness assumption
> > (it will break on big-endian), and you'd never write real code like
> > that. Just pass 'a', what's wrong with that?

In my first implementation of "%c" support, I tried to support "%lc" and
"%llc" also and reused the later int code.  Then just testing 'a' was ok,
but it was wrong with the 0x64636261, three bytes of which showed up as
part of the next %s.  So, I thought it would be better to test with int.
But exactly it breaks the big-endian case, I'll just pass 'a'.


> >
> > >         /* Overflow */
> > >         over_ret = BPF_SNPRINTF(over_out, sizeof(over_out), "%%overflow");
> > >         /* Padding of fixed width numbers */
> > > --
> > > 2.30.2
> > >