Re: strscpy

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 18/05/2020 à 19:44, Taylor R Campbell a écrit :
> > Date: Mon, 18 May 2020 18:46:15 +0200
> > From: Maxime Villard <max%m00nbsd.net@localhost>
> >
> > strscpy() would be the only advertised string copy function, with arguments
> > that match the well-known strncpy() and strlcpy() functions.
>
> Neither strscpy nor strlcpy is a substitute for strncpy.

Indeed. But I am mostly talking about 'l' -> 's'. strncpy() has specific
use-cases that probably won't disappear immediately from the kernel.

My point with the arguments, is that people are less likely to be mistaken,
since they're used to the pattern of strncpy() and strlcpy().

> For example, consider:
>
> 	struct disklabel d;
> 	strncpy(d.d_packname, "foobar", sizeof d.d_packname);
>
> This correctly initializes the entire fixed-width field with NUL bytes
> if the input is shorter than the field, rather than leaving stack
> garbage there, and does not leave an unnecessary trailing NUL byte if
> the input fills the destination field exactly.  (Yes, you could memset
> it to 0, but if it's a packed on-disk structure there should be no
> padding you have to worry about -- initializing every named field will
> initialize every byte in the object.)  Neither strlcpy nor strscpy
> will do the same.

I am well aware of the filling behavior of strncpy().

> That said, I agree that strlcpy is confusing.  I tend to think we
> should pass bounds on _both_ the destination _and_ the source: Every
> time I have ever written strlcpy or strncpy I have had to ask myself,
> `Is this the source buffer size, or the source string length, or the
> destination buffer size, and does it matter or no?', and consult the
> man page, and think about it.
>
> Maybe something like this:
>
> int
> strncpy0(char *dst, size_t ndst, const char *src, size_t nsrc)
> {
>          size_t i;
>
> 	/* If you have an empty buffer, stop before you call this.  */
> 	KASSERT(ndst > 0);
>
> 	/* Copy as many bytes as we can, but stop early at NUL.  */
>          for (i = 0; i < MIN(ndst, nsrc); i++) {
> 		if ((dst[i] = src[i]) == '\0')
> 			return 0;
> 	}
>
> 	/*
> 	 * If we ran out of destination space, overwrite the last byte
> 	 * with a NUL terminator and report truncation.  Otherwise,
> 	 * NUL-terminate in the destination space we have left and
> 	 * report success  -- we successfully copied every byte of the
> 	 * input, NUL-terminated or not.
> 	 */
> 	if (i == ndst) {
> 		dst[ndst - 1] = '\0';
> 		return ENOSPC;
> 	} else {
> 		dst[i] = '\0';
> 		return 0;
> 	}
> }
>
> That way, the usage can be:
>
> 	/* don't care if truncated, just gimme a NUL-terminated string */
> 	strncpy0(foo. sizeof foo, src, nsrc);
>
> or
>
> 	if (strncpy0(foo, sizeof foo, src, nsrc))
> 		warn("truncated");
>
> and works the same for any nsrc >= strlen(src) up to and including
> sizeof src -- so you don't _have_ to remember whether it's the source
> buffer size or the source string length because either one works.
>
> The output is guaranteed to be NUL-terminated, and ENOSPC means the
> input was truncated as a C string (but if the input was a fixed-length
> field like struct disklabel::d_typename then you can just ignore it).

I think it also matters to have a function that matches the well-established
pattern of (dst, src, len) arguments.

I believe this is why copystr(), although safer than strlcpy(), isn't used much:
the length returned in a pointer is unusual. People are just used to the
arguments of strncpy() and strlcpy(), and given that the latter is marketed as
"safe" (which it isn't), they just tend to use it.

What I find nice with strscpy(), is that it simply corrects the unsafe strlen
behavior of strlcpy(), but preserves the pattern of arguments.

As you noted, strncpy0() won't work with string literals and we'll have to fall
back to strlcpy(). I think it would be better to target every use case with one
single function. strscpy() seems like a good balance to me.

Maxime