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

[Petr Mladek]

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.


Here is the proposed clean up: