patchstrlcpy(): safer and faster version

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

> On Thu, Dec 16, 2021 at 05:31:20PM +0000, Andriy Makukha via GitGitGadget wrote:
>
>> 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).
>
>> 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.
>
>>  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).

> Jeff King <peff@peff.net> writes:
>
>> On Thu, Dec 16, 2021 at 05:31:20PM +0000, Andriy Makukha via GitGitGadget wrote:
>>
>>> 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).
>>
>>> 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.
>>
>>>  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 ;-)