strlcpy(): safer and faster version

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.

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.

Quoted 4 lines · Andriy Makukha via GitGitGadget

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 4 lines

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.

compat/strlcpy.c line 10

Quoted 9 lines

size_t gitstrlcpy(char *dest, const char *src, size_t size) {

compat/strlcpy.c +6 -1

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

Quoted 46 lines · Jeff King

Quoted 4 lines · Andriy Makukha via GitGitGadget

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 4 lines

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 12 lines

compat/strlcpy.c line 10

Quoted 9 lines

size_t gitstrlcpy(char *dest, const char *src, size_t size) {

compat/strlcpy.c +6 -1

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

Quoted 1 line

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