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 10:51:29 -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 9:57 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> I figured the best thing to do is to create a helper macro and place it
> into include/linux/string.h. And go around and fix all the open coded
> versions of it later.
>
> I plan on only applying this patch and updating the tracing hooks for
> this merge window. And perhaps use it to fix some of the bugs that were
> found.

I like the helper function concept, but as they say about CS: "There
is only one hard problem in computer science: naming and off-by-one
errors".

And this one has that problem. The name makes absolutely no sense.

Calling it "strncmp_prefix()" when there is no "n" there makes no sense.

So drop the "n" from the name.

And honestly, maybe it would be better to use a different naming
pattern entirely, and avoid the crazy non-boolean "str*cmp()" model.
That thing is useful for search comparisons (where "before/after"
matters), but it's horrible for the actual "is this true or not",
where the code ends up being

        if (!strcmp_prefix(a, "prefix")) {
                // This is the "Yes, prefix matched" case, despite the
"if !" syntax

which is just confusing.

So I'd really prefer more of a

        #define has_prefix(str, prefix) ...

model that actually returns a boolean (true if it has a prefix, false
if it doesn't), rather than use the "str*cmp" naming and return
values.

(But I agree that *if* you use the "strcmp" naming, then you do need
to hold to the traditional strcmp return value semantics).

Hmm?

Finally, I also suspect that your helper might be slightly fragile.
Doing sizeof() seems broken. I could see somebody using some prefix
define for arrays with constant sizes, and doing

   #define PFX1 "cpp\0"
   #define PFX2 "c\0\0\0"
   #define PFX3 "h\0\0\0"

or similar. Also, is there a reason you use "&prefix" for the constant
test? I don't see that pattern anywhere else. Plus it looks entirely
wrong without the parenthesis (ie "&(prefix)" to make sure we group
things right).

So a lot of small problems, but the naming one is big.

                  Linus