linux-kernel - Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Sep 2022 15:24:24 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        kernel@...gutronix.de,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

On Fri, Sep 30, 2022 at 02:14:16PM +0200, Petr Mladek wrote:
> On Fri 2022-09-30 13:10:50, Uwe Kleine-König wrote:
> > For code that emits a string representing a usual return value it's
> > convenient to have a 0 result in a string representation of success
> > instead of "00000000".
> 
> Does it really always mean success, please?
> 
> IMHO, if a function returns a pointer then typically only a valid
> pointer means success. Error code means some reasonable explanation
> of the failure. And NULL should never happen.

So your example function doesn't hit the case that we're discussing here
because it will never return NULL and so the code path I added isn't
used and doesn't make a difference, right?

> For example:
> 
> struct bla *find_bla(int key)
> {
> 	struct bla *b;
> 
> 	/* Try to get bla using the given key */
> 	...
> 
> 	if (succeded)
> 		return b;
> 
> 	/* Did not find bla for the given key */
> 	return -EINVAL;

nitpick: s/-EINVAL/ERR_PTR(-EINVAL)/

> 
> }
> 
> It might be used:
> 
> int process_bla()
> {
> 	struct bla *b;
> 
> 	b = get_bla();
> 	if (IS_ERR(b))
> 		return PTR_ERR(b);
> 
> 	/* do something with b */
> 	...
> }
> 
> If get_bla() returns NULL then it means a super fault. It means
> that get_bla() failed and did not know why.

OK, I think we agree that a function that might return an error pointer
shouldn't return NULL with the semantic "This is also an error."

Only in combination with such a function you can reasonably object the
addition of PTR_ERR(0) meaning "SUCCESS". In such a case the right
action is to fix the function.

> IMHO, this patch might do more harm than good.

Hmm, do you think there are many functions that use both NULL and
error pointers to signal a failure? I don't see where the patch might do
harm otherwise.

In *my* humble opinion it's perfectly fine that a given printk feature
results in strange output when it's fed with strange input.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)