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

[Kieran Bingham]

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.

--
Kieran




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

-- 
Regards
--
Kieran