Received-SPF: pass (google.com: domain of linux-kernel+bounces-63007-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <4de5a6a5-8f7f-4af4-a4ad-d6521b056a4f@sdfg.com.ar>
Date: Tue, 13 Feb 2024 07:20:15 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/4] tools/nolibc: Fix strlcat() return code and size
 usage
Content-Language: en-US
From: Rodrigo Campos <rodrigo@sdfg.com.ar>
To: Willy Tarreau <w@1wt.eu>
Cc: =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>,
 linux-kernel@vger.kernel.org
References: <20240129141516.198636-1-rodrigo@sdfg.com.ar>
 <20240129141516.198636-3-rodrigo@sdfg.com.ar> <20240211104817.GA19364@1wt.eu>
 <52c52665-35f5-4111-a9d4-f8669c79bf70@sdfg.com.ar>
In-Reply-To: <52c52665-35f5-4111-a9d4-f8669c79bf70@sdfg.com.ar>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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@sdfg.com.ar>
>>> ---
>>>   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.