Re: [PATCH] strlcpy(): safer and faster version

Jeff King [off-list ref] writes:

On Thu, Dec 16, 2021 at 05:31:20PM +0000, Andriy Makukha via GitGitGadget wrote:

quoted

Original strlcpy() has a significant disadvantage of being both unsafe
and inefficient. It unnecessarily calculates length of `src` which may
result in a segmentation fault if `src` is not terminated with a
NUL-character.

I think any code that passes such a "src" is still broken after your
code. If the length of "src" is less than "size", then the result in
"dest" will contain garbage we read from the memory after "src".

Likewise in that case using strnlen() isn't any faster, since it has to
look at the same number of bytes either way (it may even be slower since
its loop has two conditions to check).

quoted

In this fix, if `src` is too long, strlcpy() returns `size`. This
allows to still detect an error while fixing the mentioned
vulnerabilities. It deviates from original strlcpy(), but for a good
reason.

This could potentially break callers of strlcpy(), though, because it's
changing the semantics of the return value. For example, if they use the
return value to expand a buffer to hold the result.

I do think the proposed semantics are better (I have actually fixed a
real overflow bug where somebody assumed strlcpy() returned the number
of bytes written). But we probably should not call it strlcpy(), because
that's has well-known behavior that we're not meeting.

I don't think any of the current code would be broken by this (most does
not even look at the return value at all). It just seems like an
accident waiting to happen.

Personally, I don't love strlcpy() in the first place. Avoiding heap
overflows is good, but unexpected truncation can also be buggy. That's
why try to either size buffers automatically (strbuf, xstrfmt,
FLEX_ALLOC, etc) or assert that we didn't truncate (xsnprintf).

Some cases could probably be converted away from strlcpy(). For
instance, the color stuff in add-interactive.c should be using
xsnprintf(), since the point of COLOR_MAXLEN is to hold the
longest-possible color. The ones in difftool.c probably ought to be
strbufs. There are definitely some that want the truncation semantics
(e.g., usernames in archive-tar.c). We might be better off providing a
function whose name makes it clear that truncation is OK.

quoted

 size_t gitstrlcpy(char *dest, const char *src, size_t size)
 {
-	size_t ret = strlen(src);
+	/*
+	 * NOTE: original strlcpy returns full length of src, but this is
+	 * unsafe. This implementation returns `size` if src is too long.
+	 * This behaviour is faster and still allows to detect an issue.
+	 */
+	size_t ret = strnlen(src, size);

Also, strnlen() isn't portable, so we'd need a solution there (open
coding or yet another compat wrapper).

Thanks for saying everything I wanted to say ;-)

On Thu, Dec 16 2021, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Thu, Dec 16, 2021 at 05:31:20PM +0000, Andriy Makukha via GitGitGadget wrote:

quoted

Original strlcpy() has a significant disadvantage of being both unsafe
and inefficient. It unnecessarily calculates length of `src` which may
result in a segmentation fault if `src` is not terminated with a
NUL-character.

I think any code that passes such a "src" is still broken after your
code. If the length of "src" is less than "size", then the result in
"dest" will contain garbage we read from the memory after "src".

Likewise in that case using strnlen() isn't any faster, since it has to
look at the same number of bytes either way (it may even be slower since
its loop has two conditions to check).

quoted

In this fix, if `src` is too long, strlcpy() returns `size`. This
allows to still detect an error while fixing the mentioned
vulnerabilities. It deviates from original strlcpy(), but for a good
reason.

This could potentially break callers of strlcpy(), though, because it's
changing the semantics of the return value. For example, if they use the
return value to expand a buffer to hold the result.

I do think the proposed semantics are better (I have actually fixed a
real overflow bug where somebody assumed strlcpy() returned the number
of bytes written). But we probably should not call it strlcpy(), because
that's has well-known behavior that we're not meeting.

I don't think any of the current code would be broken by this (most does
not even look at the return value at all). It just seems like an
accident waiting to happen.

Personally, I don't love strlcpy() in the first place. Avoiding heap
overflows is good, but unexpected truncation can also be buggy. That's
why try to either size buffers automatically (strbuf, xstrfmt,
FLEX_ALLOC, etc) or assert that we didn't truncate (xsnprintf).

Some cases could probably be converted away from strlcpy(). For
instance, the color stuff in add-interactive.c should be using
xsnprintf(), since the point of COLOR_MAXLEN is to hold the
longest-possible color. The ones in difftool.c probably ought to be
strbufs. There are definitely some that want the truncation semantics
(e.g., usernames in archive-tar.c). We might be better off providing a
function whose name makes it clear that truncation is OK.

quoted

 size_t gitstrlcpy(char *dest, const char *src, size_t size)
 {
-	size_t ret = strlen(src);
+	/*
+	 * NOTE: original strlcpy returns full length of src, but this is
+	 * unsafe. This implementation returns `size` if src is too long.
+	 * This behaviour is faster and still allows to detect an issue.
+	 */
+	size_t ret = strnlen(src, size);

Also, strnlen() isn't portable, so we'd need a solution there (open
coding or yet another compat wrapper).

Thanks for saying everything I wanted to say ;-)

Isn't strlcpy() an OpenBSD-initiated effort? So if we're going to update
this at all shouldn't be be aiming for picking an "upstream" here?
E.g. [1]?

But yeah, just getting rid of it in one form or another is probably
better.

1. https://github.com/libressl-portable/openbsd/blob/master/src/lib/libc/string/strlcpy.c