Re: [PATCH 2/4] tools/nolibc: Fix strlcat() return code and size usage

[Rodrigo Campos]

On 2/13/24 00:16, Rodrigo Campos wrote:
> On 2/11/24 11:48, Willy Tarreau wrote:
> > Hi Rodrigo,
> >
> > first, thanks for the series!
>
> Thank you, for your time and review! :)
>
> > On Mon, Jan 29, 2024 at 03:15:14PM +0100, Rodrigo Campos wrote:
> > > The return code should always be strlen(src) + strlen(dst), but dst is
> > > considered shorter if size is less than strlen(dst).
> > >
> > > While we are there, make sure to copy at most size-1 bytes and
> > > null-terminate the dst buffer.
> > >
> > > Signed-off-by: Rodrigo Campos <rodrigo@xxxxxxxxxxx>
> > > ---
> > >   tools/include/nolibc/string.h | 14 +++++++-------
> > >   1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/tools/include/nolibc/string.h 
> > > b/tools/include/nolibc/string.h
> > > index ed15c22b1b2a..b2149e1342a8 100644
> > > --- a/tools/include/nolibc/string.h
> > > +++ b/tools/include/nolibc/string.h
> > > @@ -187,23 +187,23 @@ char *strndup(const char *str, size_t maxlen)
> > >   static __attribute__((unused))
> > >   size_t strlcat(char *dst, const char *src, size_t size)
> > >   {
> > > -    size_t len;
> > >       char c;
> > > +    size_t len = strlen(dst);
> > > +    size_t ret = strlen(src) + (size < len? size: len);
> >
> >  From what I'm reading in the man page, ret should *always* be the sum
> > of the two string lengths. I guess it helps for reallocation. It's even
> > explicitly mentioned:
> >
> >    "While this may seem somewhat confusing, it was done to make 
> > truncation
> >     detection simple."
>
> Yes, that was my *first* understanding of the manpage too. But it 
> doesn't seem to be the correct interpretation.
>
> Also, this is exactly what I tried to say in the cover letter, with:
>
>      I thought the manpage was clear, but when checking against that,
>      I noted a few twists (like the manpage says the return code of
>      strlcat is strlen(src) + strlen(dst), but it was not clear it is
>      not that if size < strlen(dst). When looking at the libbsd
>      implementation and re-reading the manpage, I understood what it
>      really meant).
>
>
> Sorry if I wasn't clear. Let me try again.
>
> My first interpretation of the manpage was also that, and I think it 
> would make sense to be that one. However, it is wrong, they also say 
> this, that is what made me add the ternary operator:
>
>      Note, however, that if strlcat() traverses size characters
>      without finding a NUL, the length of the string is considered
>      to be  size and the destination string will not be NUL
>      terminated (since there was no space for the NUL)
>
> So, my interpretation is that it is the sum of both, except when we 
> can't find the NUL in that size, in which case we conside "size" to be 
> the len of dst.
>
> If you compare it with the output of libbsd, the return code seems to be 
> exactly that. I was surprised too, as the manpage seem so clear... :-/


I'm not convinced now if that is the right interpretation. I'll check 
the libbsd code, I don't remember it now, it's been a few days already.

My memory is that it was not something so sane.