linux-kernel - Re: [PATCH v1 1/8] lib/string: introduce match

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

Date:	Tue, 12 Jan 2016 17:26:49 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Tejun Heo <tj@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	David Airlie <airlied@...ux.ie>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

Hello,

On (01/11/16 23:10), Rasmus Villemoes wrote:
[..]
> > Thought more about those cases.
> >
> > If you would like you may introduce something like
> >
> > int nmatch_string(array, array_size, string, int len)
> > {
> >   if (len < 0)
> >     return match_string();
> >
> >   for (...) {
> >     size_t itemlen = (len > 0) ? len : strlen(array[index]);
> > ...
> >     if (!strncmp(array[index], string, itemlen))
> >       return index;
> >   }
> >   return -EINVAL;
> > }

may be later this week; I'm a bit out of spare time at the moment.


> Yeah, a separate function is probably better. But why not a more
> explicit name, match_prefix, match_string_prefix, match_string_starts?

Not married to nmatch_string(), but at the same time, IMHO, *_prefix or
*_starts naming is not really better. One can pass a string with offset,
e.g.
  FOO_starts(array, array_size, string + offset, strlen(string) - offset)
which will be equivalent to FOO_ends(), but not FOO_starts() or FOO_prefix().
Personally, I'd prefer to preserve strcmp/strncmp semantics, thus, forbidding
`len < 0' case, which looks cryptic to me.


> I like the idea of passing the string length if one wants the "is this a
> prefix of some array element" semantics, and a sentinel otherwise. But I
> don't see any case where one would want match_string() semantics (why
> not call match_string directly instead?),


> so why not let len < 0 mean "is some array element a prefix of this string"
> and "len >= 0" be the other case. I don't see why one shouldn't be able to
> ask "is the empty string a prefix of some array element" (that is, are there
> any elements in the array);

if this is a dynamic array, then there should be some function that
fills in that array, so having a simple bool flag in the code will
suffice; if this is a static array, then ARRAY_SIZE() should do the
trick. I would never expect a string matching function to have this
type of functionality, TBH.

But the question is

> is the empty string a prefix of some array element

do people really need this?

the way I see it, the idea is to have wrappers around
	while (array[..]) if strcmp()/strncmp() == 0 break ...

both of which [strcmp()/strncmp()] have a well known and expected
semantics, changing this can only confuse people.

	-ss

> both the array and the string might be run-time things,
> so this could occur. And it's not up to a generic library routine like
> this to impose restrictions like "the empty string makes no sense, go
> away".
> 
> Rasmus
>