Re: strscpy

[Taylor R Campbell <campbell+netbsd-tech-security%mumble.net@localhost>]

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


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

The example that prompted this in if_pppoe.c,

	strlcpy(error, mtod(n, char *) + noff, len + 1);

would become:

	/* Copy a possibly non-NUL-terminated field into a C string.  */
	strncpy0(error, len + 1 /* same amount as passed to malloc above */,
	    mtod(n, char *) + noff,
	    len /* same amount as passed to m_pulldown above */);

Of course, we're using strnvis there now rather than copying the
string verbatim since it gets printed to the console, so this case is
moot, but it serves to illustrate how to audit correct uses.

strncpy0 is a less convenient for literal source strings, but those
are always safe with strlcpy and it's easy to audit strlcpy for
literal vs nonliteral source strings.  (We could even reinterpret
`strlcpy' to mean `string literal copy' and forbid it for non-literal
strings!)