On Thu, Dec 16 2021, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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 ;-)
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
