From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Al Viro <viro@ZenIV.linux.org.uk>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/8] lib: string: Introduce strreplace
Organization: D03
References: <1433806017-10823-1-git-send-email-linux@rasmusvillemoes.dk>
	<1433806017-10823-2-git-send-email-linux@rasmusvillemoes.dk>
	<1433808048.2730.46.camel@perches.com>
Date: Tue, 09 Jun 2015 09:17:41 +0200
In-Reply-To: <1433808048.2730.46.camel@perches.com> (Joe Perches's message of
	"Mon, 08 Jun 2015 17:00:48 -0700")
Message-ID: <87pp55jqp6.fsf@rasmusvillemoes.dk>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1955
Lines: 52

On Tue, Jun 09 2015, Joe Perches <joe@perches.com> wrote:

> On Tue, 2015-06-09 at 01:26 +0200, Rasmus Villemoes wrote:
>> Strings are sometimes sanitized by replacing a certain character
>> (often '/') by another (often '!').
> []
>> v2: spello fixed, parameters renamed 'old' and 'new' (just so the
>> kernel doc aligns nicely, and because that's what python -c
>> 'help(str.replace)' uses). Still EXPORT_SYMBOL, not inline (tried it,
>> caused more bloat), still called strreplace.
>
> OK, thanks.  I think the chars should be ints though
> just for consistency for strchr variants.

I disagree. That way lies subtle (semi)bugs. Quick quiz: What does
memscan return below?

  char a[1] = {X}; /* for some suitable X */
  char *p = memscan(a, a[0], 1);

Here's the kerneldoc+prototype for memscan:

/**
 * memscan(void *addr, int c, size_t size) - Find a character in an area of memory.
 * @addr: The memory area
 * @c: The byte to search for
 * @size: The size of the area.
 *
 * returns the address of the first occurrence of @c, or 1 byte past
 * the area if @c is not found
 */

So obviously p==a, right? Wrong. Or rather, wrong when char is signed
and X lies outside the ascii range. Or maybe right, if you're on an
architecture with its own memscan that DTRT. And 'the right thing' is
obviously to use only the LSB of c, which would have been harder to get
wrong if c was just a u8 to begin with.

(The only in-tree callers which do not pass an explicit non-negative
constant seem to be in drivers/hid/usbhid/usbkbd.c and
net/bluetooth/hidp/core.c, and they both pass something from an unsigned
char array).

We're stuck with int in the libc functions, but we can do better for new
interfaces.

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