Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Ilya Dryomov]

On Wed, Feb 19, 2020 at 4:40 PM Rasmus Villemoes
<linux@xxxxxxxxxxxxxxxxxx> wrote:
>
> On 19/02/2020 15.45, Petr Mladek wrote:
> > On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote:
> >> On 19/02/2020 14.48, Petr Mladek wrote:
> >>> On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote:
> >>>> --- a/lib/vsprintf.c
> >>>> +++ b/lib/vsprintf.c
> >>> The test should go into null_pointer() instead of errptr().
> >>
> >> Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that
> >> way. But I should add a #else section that tests how %pe behaves without
> >> CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch.
> >
> > OK, we should agree on some structure first.
> >
> > We already have two top level functions that test how a particular
> > pointer is printed using different pointer modifiers:
> >
> >       null_pointer();     -> NULL with %p, %pX, %pE
> >       invalid_pointer();  -> random pointer with %p, %pX, %pE
> >
> > Following this logic, errptr() should test how a pointer from IS_ERR() range
> > is printed using different pointer formats.
>
> Oh please. I wrote test_printf.c originally and structured it with one
> helper for each %p<whatever>. How are your additions null_pointer and
> invalid_pointer good examples for what the existing style is?
>
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 649)
> test_pointer(void)
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 650) {
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 651)  plain();
> 3e5903eb9cff7 (Petr Mladek      2019-04-17 13:53:48 +0200 652)
> null_pointer();
> 3e5903eb9cff7 (Petr Mladek      2019-04-17 13:53:48 +0200 653)
> invalid_pointer();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 654)
> symbol_ptr();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 655)
> kernel_ptr();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 656)
> struct_resource();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 657)  addr();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 658)
> escaped_str();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 659)
> hex_string();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 660)  mac();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 661)  ip();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 662)  uuid();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 663)  dentry();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 664)
> struct_va_format();
> 4d42c44727a06 (Andy Shevchenko  2018-12-04 23:23:11 +0200 665)
> struct_rtc_time();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 666)
> struct_clk();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 667)  bitmap();
> 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 668)
> netdev_features();
> edf14cdbf9a0e (Vlastimil Babka  2016-03-15 14:55:56 -0700 669)  flags();
> 57f5677e535ba (Rasmus Villemoes 2019-10-15 21:07:05 +0200 670)  errptr();
> f1ce39df508de (Sakari Ailus     2019-10-03 15:32:19 +0300 671)
> fwnode_pointer();
>
>
> > I am open to crate another logic but it must be consistent.
>
> So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM.
>
> > If you want to check %pe with NULL in errptr(), you have to
> > split the other two functions per-modifier. IMHO, it is not
> > worth it.
>
> Agreed, let's leave null_pointer and invalid_pointer alone.
>
> >>>> BTW., your original patch for %p lacks corresponding update of
> >>>> test_vsprintf.c. Please add appropriate test cases.
> >>>
> >>> diff --git a/lib/test_printf.c b/lib/test_printf.c
> >>> index 2d9f520d2f27..1726a678bccd 100644
> >>> --- a/lib/test_printf.c
> >>> +++ b/lib/test_printf.c
> >>> @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p)
> >>>  static void __init
> >>>  null_pointer(void)
> >>>  {
> >>> -   test_hashed("%p", NULL);
> >>> +   test(ZEROS "00000000", "%p", NULL);
> >>
> >> No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases
> >> (where one of course has to use explicit integers and not E* constants).
> >
> > Yes, it would be great to add checks for %p, %px for IS_ERR() range.
> > But it is different story. The above change is for the original patch
> > and it was about NULL pointer handling.
>
> Wrong. The original patch (i.e. Ilya's) had subject "vsprintf: don't
> obfuscate NULL and error pointers" and did
>
> +       if (IS_ERR_OR_NULL(ptr))
>
> so the tests that should be part of that patch very much need to cover
> both NULL and ERR_PTRs passed to plain %p.

I sent v2 of my patch with the update to test_printf.c.

I see your point about one function for each %p variant, but since
it's already been disrupted with null_pointer() and invalid_pointer()
and also because test_hashed() has a comment which implies that it
must be called after plain(), I piled on by adding error_pointer().

Thanks,

                Ilya