linux-kernel - Re: [patch 04/91] proc: save LOC in __xlate_proc

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-next>] [day] [month] [year] [list]

Date:   Fri, 7 May 2021 08:24:33 +0300
From:   Alexey Dobriyan <adobriyan@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [patch 04/91] proc: save LOC in __xlate_proc_name()

On Thu, May 06, 2021 at 07:24:36PM -0700, Linus Torvalds wrote:
> On Thu, May 6, 2021 at 6:02 PM Andrew Morton <akpm@...ux-foundation.org> wrote:
> >
> > From: Alexey Dobriyan <adobriyan@...il.com>
> > Subject: proc: save LOC in __xlate_proc_name()
> ..
> > +       while ((next = strchr(cp, '/'))) {
> 
> Please don't do this.

It is actually how it should be done.

Kernel has such code in other places

	#define hlist_for_each(pos, head) \
	        for (pos = (head)->first; pos ; pos = pos->next)

And we do check pointers for validness like this

	if (ptr) {
	}

"while" loop is no different.

> Yes, gcc suggests that double parentheses syntax around an assignment
> to avoid warnings.

I never saw this warning. I just wrote double parenth knowing the
warning will be emitted. It's an old warning.

> gcc is wrong, and is being completely stupid.

> The proper way to avoid the "assignment in conditional" warning is to
> (surprise, surprise) USE A CONDITIONAL.
> 
> So that
> 
>           while ((next = strchr(cp, '/'))) {
> 
> is the crazy rantings of a misguided compiler. No sane human should
> ever care about some odd double parenthesis syntax. We're not writing
> LISP, for chrissake.
> 
> The proper way to write this is
> 
>           while ((next = strchr(cp, '/')) != NULL) {

This NULL is redundant in the same way "if (ptr != NULL)" is redundant.
Even more so in C where comparison can't be overloaded.
You don't even save the parenth.

> which makes sense to not just a machine, but to a human, and avoids
> the whole "assignment used as a conditional" warning very naturally.
> 
> See? Now it uses a conditional as a conditional. Doesn't that make a
> whole lot more sense than the crazy ramblings of a broken machine
> mind?
> 
> I fixed it up manually, I just wanted to rant against this kind of
> "mindlessly take advice from the compiler without thinking about it".

Whatever-by: Alexey Dobriyan <adobriyan@...il.com>