linux-kernel - Re: [for-next][PATCH 23/24] string.h: Add strncmp

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, 21 Dec 2018 14:08:11 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Joe Perches <joe@...ches.com>,
        Tom Zanussi <zanussi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

On Fri, Dec 21, 2018 at 12:55 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Fri, 21 Dec 2018 12:41:17 -0800
> Linus Torvalds <torvalds@...ux-foundation.org> wrote:
>
> > Parentheses....
>
> ?

Your patch actually had them, but in the body of your email you did

> #define have_prefix(str, prefix) ({ \
>       const char *__pfx = (const char *)prefix; \

which is just completely wrong.

Considering your _old_ patch had the exact same bug, I really think
you need to start internalizing the whole "macro arguments *have* to
be properly protected" thing.

And I agree with Joe that using a million underscores just makes code
less legible. Two underscores at the beginning is the standard
namespace protection. Underscores at the end do nothing. And using
*more* than two is just crazy.

Finally, I think the cast is actually bogus and wrong. Why would the
prefix ever be anything but "const char *"? Adding the cast only makes
it more possible that somebody uses a truly wrong type ("unsigned long
*" ?), and then the cast just silently hides it.

If somebody uses "unsigned char *" for this, they'd get the exact same
warning if they were using strncmp and do this by hand.

So why would that helper function act any differently? Particularly
when it then has the huge downside that it will also accept absolute
garbage?

Anyway, that was a long and winding NAK for your patch.

             Linus