Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Geert Uytterhoeven]

Hi Petr,

On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek <pmladek@xxxxxxxx> wrote:
> On Mon 2018-02-05 21:58:17, Adam Borowski wrote:
>> On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
>> > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding <me@xxxxxxxx> wrote:
>> > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
>> > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
>> > >> >> Like %pK already does, print "00000000" instead.
>> > >> >>
>> > >> >> This confused people -- the convention is that "(null)" means you tried to
>> > >> >> dereference a null pointer as opposed to printing the address.
>> > >
>> > > Leaving aside what is converting to %px.  If we consider that using %px
>> > > is meant to convey to us that we _really_ want the address, in hex hence
>> > > the 'x', then it is not surprising that we will get "00000000"'s for a
>> > > null pointer, right?  Yes it is different to before but since we are
>> > > changing the specifier does this not imply that there may be some
>> > > change?
>> >
>> > I personally prefer 0000s, but if we're going to change this, we need
>> > to be aware of the difference.
>>
>> It's easy to paint this bikeshed any color you guys want to: there's an "if"
>> already.  My preference is also 0000; NULL would be good, too -- I just
>> don't want (null) as that has a special meaning in usual userspace
>> implementations; (null) also fits well most other modes of %p as they show
>> some object the argument points to.  Confusion = wasted debugging time.
>
>> Let's recap:
>>
>> Currently:
>>               not-null              null
>> %pponies      object's description  (null)
>> %px           address               (null)
>> %pK           hash                  hash
>>
>> I'd propose:
>>               not-null              null
>> %pponies      object's description  (null)
>> %px           address               00000000
>> %pK           hash                  00000000
>
> It makes sense to me[*], so
>
> Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>
>
> It seems that all people agree with this change. But there was also some
> confusion. I am going to give it few more days before I push it to
> Linus. It means waiting for 4.16-rc3 because I will be without
> reliable internet next week. Anyone, feel free to push it faster.
>
>
> [*] I made some archaeology:
>
> The "(null)" string was added by the commit d97106ab53f812910
> ("Make %p print '(null)' for NULL pointers").
>
> It was a generic solution to prevent eventual crashes, see
> https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xyzzy@xxxxxxxxxxxxx
>
> From this point, printing 00000000 for %px looks perfectly fine because
> it does not crash.
>
> In fact, it would have made perfect sense to print 00000000 for pure
> %p because it did not crash. But nobody has cared about the eventual
> confusion yet.
>
> I am not sure if it makes sense to change the pure %p handling
> now. Note that printing "(null)" has the advantage that we
> get this string instead of the hash ;-)

Note that "(null)" is also used for printing strings, where you do dereference
the pointer, unlike for printing pointers.
In addition, "(null)" for strings is not just printed for real NULL
pointers, but
also for anything pointing within the first page of virtual memory.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds