linux-kernel - Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function

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]

Message-ID: <87d2b1jjqa.fsf@rasmusvillemoes.dk>
Date:	Fri, 12 Sep 2014 11:01:17 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Grant Likely <grant.likely@...aro.org>,
	Andi Kleen <ak@...ux.intel.com>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	"H. Peter Anvin" <hpa@...ux.intel.com>,
	linux-kernel@...r.kernel.org, Joe Perches <joe@...ches.com>,
	Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function

On Fri, Sep 12 2014, Andrew Morton <akpm@...ux-foundation.org> wrote:

> On Wed, 27 Aug 2014 09:36:01 +0200 Rasmus Villemoes <linux@...musvillemoes.dk> wrote:
>
>> lib/string.c contains two functions, strnicmp and strncasecmp, which
>> do roughly the same thing, namely compare two strings
>> case-insensitively up to a given bound. They have slightly different
>> implementations, but the only important difference is that strncasecmp
>> doesn't handle len==0 appropriately; it effectively becomes strcasecmp
>> in that case. strnicmp correctly says that two strings are always
>> equal in their first 0 characters.
>> 
>> strncasecmp is the POSIX name for this functionality. So rename the
>> non-broken function to the standard name. To minimize the impact on
>> the rest of the kernel (and since both are exported to modules), make
>> strnicmp a wrapper for strncasecmp.
>
> I guess it's safe to assume that nobody was depending on the
> strncasecmp() bug.

Hm, I thought so as well, but decided to double check. I found one minor
issue; maybe Tejun can tell if my analysis is correct.

In drivers/ata/libata-core.c, ata_parse_force_one(), it is not
immediately clear to me that val cannot end up being the empty
string. With the buggy strncasecmp, the continue branch is always
followed (since fp->name is not empty); however, with strncasecmp with
the correct semantics, the empty string is obviously a prefix of every
fp->name. So even though the comment says that "1.5" is an ok
abbreviation of "1.5Gbps", I don't think the intention was to allow ""
to be an abbreviation of everything. Anyway, the worst that can happen
seems to be that "ambigious value" [sic] becomes the *reason instead
of "unknown value".

I may have overlooked similar issues; my checking was basically "is the
length parameter an explicit non-zero number or strlen() of some static
data in some table (where I assume empty strings do not lurk)".

> Yes, please prepare the strnicmp()->strncasecmp() patches and let's get
> them merged up.  After a kernel release or two we can zap the
> back-compat wrapper.

Will do. Is there a fixed SHA1 I can refer to in the commit logs?

> And it isn't just "out of tree modules" that we should be concerned
> about - we'll commonly find that "to be in tree" code is using
> interfaces which we're trying to alter or remove, so it takes a cycle
> or two to get everything propagated.  Most of this code can be found in
> linux-next.

Would it make sense to add a checkpatch warning to ensure new users
are not introduced, and then remove it when the wrapper is also removed?

Thanks,
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/