[libcamera-devel] [PATCH v6 1/5] libcamera: utils: Add strlcpy

[Kieran Bingham]

Hi Laurent,

On 06/01/2020 17:07, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Jan 06, 2020 at 09:38:10AM +0000, Kieran Bingham wrote:
>> Hi Paul, Laurent,
>>
>> Only discussion here, as I see this patch is merged (which is where I
>> saw it and it caught my attention)
>>
>> On 04/01/2020 00:22, Laurent Pinchart wrote:
>>> Hi Paul,
>>>
>>> Thank you for the patch.
>>>
>>> On Fri, Jan 03, 2020 at 07:14:09PM -0500, Paul Elder wrote:
>>>> strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.
>>>> Instead of checking for strlcpy availability and modifying dependencies,
>>>> implement it in utils, as a wrapper around strncpy.
>>
>> Oh wow, I just had a fun read up on this topic [0], [1]. People get
>> really picky about string handling :D
>>
>> [1] highlights the following:
>>
>>> The primary complaint about strlcpy() is that it gives no indication
>>> of whether the copied string was truncated or not. So careless code
>>> using strlcpy() risks replacing a buffer-overrun error with a
>>> different problem resulting from corrupted strings. In the minds of
>>> many developers, strlcpy() just replaces one issue with another and,
>>> thus, does not solve the problem of safe string handling in C
>>> programs. They believe it should not be used, and, since it should
>>> not be used, it also should not be provided by the library.
>>
>> Personally, I disagree with the "Should not be provided by the library"
>> on the basis which is also discussed that now there are N variable
>> implementations all with their own bugs.
>>
>> That said, I don't mind adding this in here, as long as we make sure
>> we're careful with it, thus see below:
>>
>> [0] The ups and downs of strlcpy() (Michael Kerrisk)
>>     https://lwn.net/Articles/507319/
>>
>> [1] Adding strlcpy() to glibc (Jonathon Corbet)
>>     https://lwn.net/Articles/612244/
>>
>>>> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>>>>
>>>> ---
>>>> New in v6
>>>> ---
>>>>  src/libcamera/include/utils.h |  3 +++
>>>>  src/libcamera/utils.cpp       | 19 +++++++++++++++++++
>>>>  2 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
>>>> index a80f7d09..badc7753 100644
>>>> --- a/src/libcamera/include/utils.h
>>>> +++ b/src/libcamera/include/utils.h
>>>> @@ -12,6 +12,7 @@
>>>>  #include <memory>
>>>>  #include <ostream>
>>>>  #include <string>
>>>> +#include <string.h>
>>>>  #include <sys/time.h>
>>>>  
>>>>  #define ARRAY_SIZE(a)	(sizeof(a) / sizeof(a[0]))
>>>> @@ -112,6 +113,8 @@ inline _hex hex<uint64_t>(uint64_t value, unsigned int width)
>>>>  }
>>>>  #endif
>>>>  
>>>> +size_t strlcpy(char *dst, const char *src, size_t size);
>>>> +
>>>>  } /* namespace utils */
>>>>  
>>>>  } /* namespace libcamera */
>>>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
>>>> index d632f6e6..2d6e082e 100644
>>>> --- a/src/libcamera/utils.cpp
>>>> +++ b/src/libcamera/utils.cpp
>>>> @@ -182,6 +182,25 @@ operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const _hex
>>>>   * otherwise. The \a os stream configuration is not modified.
>>>>   */
>>>>  
>>>> +/**
>>>> + * \brief Copy a string with a size limit
>>>> + * \param[in] dst The destination string
>>>> + * \param[in] src The source string
>>>> + * \param[in] size The size of the destination string
>>>> + *
>>>> + * strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.
>>>> + * Instead of checking for strlcpy availability and modifying dependencies,
>>>> + * it is implemented here as a wrapper around strncpy.
>>>
>>> Shouldn't you instead document what the function does ?
>>>
>>>  * This function copy the null-terminated string \a src to \a dst with a limit>  * of \a size - 1 characters, and null-terminates the result if \a size is
>>>  * larger than 0. If \a src is larger than \a size - 1, \a dst is truncated.
>>>
>>
>> "Users should check the return value of this call against the size of
>> the destination buffer to determine if truncation occurs." ?
>>
>>>> + *
>>>> + * \return The size of \a src
>>>> + */
>>>> +size_t strlcpy(char *dst, const char *src, size_t size)
>>>> +{
>>>
>>> 	if (size) {
>>>
>>>> +	strncpy(dst, src, size);
>>>> +	dst[size - 1] = '\0';
>>>
>>> 	}
>>>
>>> just to be safe ?
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>
>>>> +	return strlen(src);
>>
>> So the main thing that caught my eye here was the strlen(src) (rather
>> than dst) here, which is because strlcpy returns the length of the
>> source which must be checked against for truncation.
>>
>> Should we bring in some compiler attributes to ensure the return value
>> is always checked?
>>
>> https://elixir.bootlin.com/linux/v5.5-rc5/source/include/linux/compiler_types.h#L108
>> ?
>>
>> Saying that, though - I see that the only uses of this call (so far) do
>> not check the return value or validate against truncation. Perhaps we're
>> happy with that, but we do so silently.
>>
>> Of course in those instances though - we ought to know the length of the
>> source anyway as it is defined by V4L2 I believe.
> 
> In this specific case the idea is to indeed truncate. We silently ignore
> truncation because there's nothing we can do about it, the length of the
> buffers is defined by the V4L2 APÏ, and they're small enough that
> truncation happens quite often in existing kernel drivers. This is a bad
> kernel API, there's not much we can do about it :-(

Ok - no problem then.
--
Kieran



>>>> +}
>>>> +
>>>>  } /* namespace utils */
>>>>  
>>>>  } /* namespace libcamera */
> 

-- 
Regards
--
Kieran