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

[Petr Mladek]

On Thu 2020-02-20 16:02:48, Ilya Dryomov wrote:
> On Thu, Feb 20, 2020 at 1:57 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
> >
> > On Wed 2020-02-19 16:40:08, Rasmus Villemoes 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?
> >
> > I see, I was the one who broke the style. Please, find below a patch
> > that tries to fix it. If you agree with the approach then I could
> > split it into smaller steps.
> >
> > Also it would make sense to add checks for NULL and ERR pointer
> > into each existing %p modifier check. It will make sure that
> > check_pointer() is called in all handlers.
> >
> >
> > > So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM.
> >
> > OK.
> >
> > > >>>> 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.
> >
> > Grr, I see. I was too fast yesterday. OK, I suggest to fix the
> > structure of the tests first. All these patches are for 5.7
> > anyway.
> 
> My patch fixes a regression introduced by 3e5903eb9cff ("vsprintf:
> Prevent crash when dereferencing invalid pointers" in 5.2, which
> made debugging based on existing pr_debugs (used extensively in some
> subsystems) very annoying.
> 
> I would like to see it in 5.6, so that it is backported to 5.4 and 5.5.

OK, it would make sense to make the patch minimalist to make it
easier for backporting.


> Please note that I sent v2 of my patch ("[PATCH v2] vsprintf: don't
> obfuscate NULL and error pointers"), fixing null_pointer() and adding
> error_pointer() test cases, which conflicts with this restructure.

IMHO, v2 creates even more mess in print tests that would need
to be fixed later.

If we agree to have a minimalist patch for backport
then I suggest to take v1. We could clean up and update
tests later.

Rasmus, others, is anyone against this approach (v1 first,
tests later)?

Best Regards,
Petr