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  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 18:50:55 +0900
From:   Sergey Senozhatsky <>
To:     Rasmus Villemoes <>
Cc:     Sergey Senozhatsky <>,
        Andrew Morton <>,
        Arnd Bergmann <>, Martin Wilck <>,
        Greg Kroah-Hartman <>,
        Andy Shevchenko <>,,
        Sergey Senozhatsky <>
Subject: Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and

Hi Rasmus,

On (08/21/18 09:59), Rasmus Villemoes wrote:
> > +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.

I think that sysfs input is always properly NULL-terminated. It may or
may not contain \n, but \0 is expected to be there. Am I 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.

The reason I added both strncpy() and strlcpy() was that there are lots
of sysfs ->store() callbacks which use strncpy().

	and so on and on.

> > +	c = dest + count - 1;
> > +	while (c >= dest && (isspace(*c) || *c == '\n' || *c == '\0')) {
> nit: '\n' certainly already passes the isspace() test.

Hah, indeed, it should.
"\n" & 0x20 must be positive. Andrew had the same comment. But I didn't
check what actually isspace() was doing, until now.

> > +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?

Honestly, I didn't think about it. I wasn't sure if we want to return
anything from this function and from sysfs_strncpy(). I glanced through
a number of ->store() callbacks, and it seems that mostly people don't
bother to check strlcpy() return value at all.

> 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.

Going point, that's why the patch is in RFC stage: to figure out
what do we actually want.

> 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?

I like it in general. Sounds like a plan to me. Maybe the "-EOVERFLOW if
some none-whitespace character was chopped" part can be changed: if we
would trim leading and trailing whitespaces before we copy a string then
only valid input chars can get chopped.


Powered by blists - more mailing lists