linux-kernel - Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs

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:   Tue, 21 Aug 2018 09:59:42 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Arnd Bergmann <arnd@...db.de>, Martin Wilck <mwilck@...e.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and
 sysfs_strlcpy()

On 2018-08-21 08:24, Sergey Senozhatsky wrote:
> +/**
> + * sysfs_strncpy - Trim a length-limited C-string (wgutesoaces and a trailing
> + *                 newline symbol) and copy into a buffer
> + * @dest: Where to copy the string to
> + * @src: Where to copy the string from
> + * @count: The maximum number of bytes to copy
> + *
> + * A wrapper around strncpy().
> + *
> + */
> +char *sysfs_strncpy(char *dest, const char *src, size_t count)
> +{
> +	char *c;
> +
> +	strncpy(dest, skip_spaces(src), count);

I'd like to see where and how you'd use this, but I'm very skeptical of
count being used both for the size of the dest buffer as well as an
essentially random argument to strncpy - if count is also the maximum
number of bytes to read from the src, you'd need to take the
skip_spaces() into account, because there are not count bytes left after
that... And if src is not necessarily nul-terminated, skip_spaces() by
itself is wrong.

Moreover, I don't think we should add more users or wrappers for strncpy
- I highly doubt the sysfs users you have in mind want the "fill the
rest of the buffer with '\0'" nor the "not enough room for a terminating
'\0'? Oh well, what could possibly go wrong" semantics.

> +	c = dest + count - 1;
> +	while (c >= dest && (isspace(*c) || *c == '\n' || *c == '\0')) {

nit: '\n' certainly already passes the isspace() test.

> +		*c = '\0';
> +		c--;
> +	}
> +	return dest;
> +}
> +EXPORT_SYMBOL(sysfs_strncpy);
> +
> +/**
> + * sysfs_strlcpy - Trim a C-string (whitespaces and a trailing newline symbol)
> + *                 and copy it into a sized buffer
> + * @dest: Where to copy the string to
> + * @src: Where to copy the string from
> + * @size: size of destination buffer
> + *
> + * A wrapper around strlcpy().
> + *
> + */
> +size_t sysfs_strlcpy(char *dest, const char *src, size_t size)
> +{
> +	size_t ret;
> +	char *c;
> +
> +	ret = strlcpy(dest, skip_spaces(src), size);
> +
> +	size = strlen(dest);
> +	c = dest + size - 1;
> +	while (c >= dest && (isspace(*c) || *c == '\n'))
> +		c--;
> +	*(c + 1) = '\0';
> +	return ret;
> +}

What exactly is the return value? The string length of the src after
skipping leading whitespace? What does that really have to do with the
string now in dest, and what is the user supposed to do with it? A more
useful return value would either be "the length of the string now in
dest", or some sort of indicator that the input was truncated, if that
is ever possible.

I think you're too focused on making wrappers around str[ln]cpy
preserving parts of those functions' API. Instead, try to figure out
what sysfs users actually want, name the functions after that, and then
whether they use strncpy or sprintf or strscpy internally is completely
irrelevant.

Maybe

int strcpy_trim(char *dst, size_t dstsize, const char *src, size_t
srcsize) - copy (potentially not '\0'-terminated) src to dst, trimming
leading and trailing whitespace. dstsize must be positive, and dst is
guaranteed to be '\0'-terminated. Returns the length of the string now
in dst, or -EOVERFLOW if some none-whitespace character was chopped.

would cover all use cases?

Rasmus