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

[Laurent Pinchart]

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 :-(

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

-- 
Regards,

Laurent Pinchart