Re: [PATCH 1/2] Implement strlcpy and strlcat [BZ #178]

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2023-04-20 06:55, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>>> index 673cfd7272..0c78ad2539 100644
>>> --- a/include/string.h
>>> +++ b/include/string.h
>>> @@ -88,6 +88,10 @@ libc_hidden_proto (__stpcpy)
>>>    # define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
>>>    #endif
>>>    libc_hidden_proto (__stpncpy)
>>> +extern __typeof (strlcpy) __strlcpy;
>>> +libc_hidden_proto (__strlcpy)
>>> +extern __typeof (strlcat) __strlcat;
>>> +libc_hidden_proto (__strlcat)
>>>    libc_hidden_proto (__rawmemchr)
>>>    libc_hidden_proto (__strcasecmp)
>>>    libc_hidden_proto (__strcasecmp_l)
>>
>> Do we want to delay doing this until we have an actual internal use of
>> these interfaces?
> 
> The *_chk functions need these aliases today.

Ack.

>>> +__fortify_function size_t
>>> +__NTH (strlcat (char *__restrict __dest, const char *__restrict __src,
>>> +		size_t __n))
>>> +{
>>> +  if (__glibc_objsize (__dest) != (size_t) -1
>>> +      && (!__builtin_constant_p (__n > __glibc_objsize (__dest))
>>> +	  || __n > __glibc_objsize (__dest)))
>>> +    return __strlcat_chk (__dest, __src, __n, __glibc_objsize (__dest));
>>> +  return __strlcat_alias (__dest, __src, __n);
>>> +}
>>> +#endif /* __USE_MISC */
>>> +
>>
>> Couldn't we use the __glibc_fortify macros here?
> 
> Do you have a concrete proposal?  I was just following what we have for
> stpncpy and other functions.

I don't, but I'll give it a shot.

> I don't think it's possible to use the generic macros for
> wcslcpy/wcslcat because of the bytes vs wide characters distinction.

Hmm, there's __glibc_fortify_n for wide chars which __wcsncpy_chk uses, 
or have I misunderstood your comment?

>>> +size_t
>>> +__strlcpy (char *__restrict dest, const char *__restrict src, size_t size)
>>> +{
>>> +  size_t src_length = strlen (src);
>>> +
>>> +  if (__glibc_unlikely (src_length >= size))
>>> +    {
>>> +      if (size > 0)
>>> +	{
>>> +	  /* Copy the leading portion of the string.  The last
>>> +	     character is subsequently overwritten with the NUL
>>> +	     terminator, but the destination size is usually a
>>> +	     multiple of a small power of two, so writing it twice
>>> +	     should be more efficient than copying an odd number of
>>> +	     bytes.  */
>>> +	  memcpy (dest, src, size);
>>> +	  dest[size - 1] = '\0';
>>> +	}
>>> +    }
>>> +  else
>>> +    /* Copy the string and its terminating NUL character.  */
>>> +    memcpy (dest, src, src_length + 1);
>>
>> size == 0 is undefined anyway; we return without touching the dest
>> because that's convenient for us.  OK.
> 
> size == 0 is defined in OpenBSD via snprintf equivalence, but maybe
> that's over-interpreting the manual page.

Doesn't seem like over-interpreting to me:

"The strlcpy() and strlcat() functions copy and concatenate strings with 
the same input parameters and output result as snprintf(3)"

We should probably just pick a lane then (and specify it in the manual), 
and snprintf equivalence is probably a reasonable take in terms of safety.

Thanks,
Sid