public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Carlos O'Donell <codonell@redhat.com>
To: Florian Weimer <fw@deneb.enyo.de>,
	libc-alpha@sourceware.org, Rich Felker <dalias@libc.org>
Subject: Re: [review] manual: Clarify strnlen, wcsnlen, strndup null termination behavior
Date: Thu, 28 Nov 2019 15:58:00 -0000	[thread overview]
Message-ID: <4312fc25-9b4c-9fab-25b1-495a618ab132@redhat.com> (raw)
In-Reply-To: <0373aed6-d865-14ef-d106-a1c2b4d06c8c@redhat.com>

On 11/28/19 10:56 AM, Carlos O'Donell wrote:
> On 11/28/19 4:43 AM, Florian Weimer wrote:
>> * Florian Weimer:
>>
>>> * Andreas Schwab:
>>>
>>>> On Okt 30 2019, Florian Weimer wrote:
>>>>
>>>>> * Andreas Schwab:
>>>>>
>>>>>> On Okt 30 2019, Florian Weimer (Code Review) wrote:
>>>>>>
>>>>>>> +Note that @var{s} must be an array of at least @var{maxlen} bytes.  It
>>>>>>> +is undefined to call @code{strnlen} on a shorter array, even if it is
>>>>>>> +known that the shorter array contains a null terminator.
>>>>>>
>>>>>> This is not true.  strnlen _always_ stops before the null byte.
>>>>>
>>>>> This is not how it is specified in POSIX.
>>>>
>>>> Yes, it is.
>>>>
>>>>     The strnlen() function shall return the number of bytes preceding
>>>>     the first null byte in the array to which s points, if s contains a
>>>>     null byte within the first maxlen bytes; otherwise, it shall return
>>>>     maxlen.
>>>>
>>>> There is nothing undefined here.  Your interpretation would be
>>>> completely useless anyway.
>>>
>>> It says “array”, which implies a length.  Admittedly, it does not say
>>> that maxlen corresponds to the arrray length.  POSIX also says this:
>>>
>>> | The strnlen() function shall never examine more than maxlen bytes of
>>> | the array pointed to by s.
>>>
>>> But it does NOT say that reading stops after the first null terminator.
>>
>> I have built glibc with --disable-multi-arch and this patch on x86-64:
>>
>> diff --git a/string/strnlen.c b/string/strnlen.c
>> index 0b3a12e8b1..d5781dbb6f 100644
>> --- a/string/strnlen.c
>> +++ b/string/strnlen.c
>> @@ -33,6 +33,10 @@
>>  size_t
>>  __strnlen (const char *str, size_t maxlen)
>>  {
>> +  /* Assert that the entire input is readable.  */
>> +  for (size_t i = 0; i < maxlen; ++i)
>> +    asm volatile ("" :: "r" (str[i]));
>> +
>>    const char *char_ptr, *end_ptr = str + maxlen;
>>    const unsigned long int *longword_ptr;
>>    unsigned long int longword, himagic, lomagic;
>> diff --git a/sysdeps/x86_64/strnlen.S b/sysdeps/x86_64/strnlen.S
>> deleted file mode 100644
>> index d3c43ac482..0000000000
>> --- a/sysdeps/x86_64/strnlen.S
>> +++ /dev/null
>> @@ -1,6 +0,0 @@
>> -#define AS_STRNLEN
>> -#define strlen __strnlen
>> -#include "strlen.S"
>> -
>> -weak_alias (__strnlen, strnlen);
>> -libc_hidden_builtin_def (strnlen)
>> diff --git a/wcsmbs/wcsnlen.c b/wcsmbs/wcsnlen.c
>> index 17e004dcc0..0d3709ac91 100644
>> --- a/wcsmbs/wcsnlen.c
>> +++ b/wcsmbs/wcsnlen.c
>> @@ -26,6 +26,10 @@
>>  size_t
>>  __wcsnlen (const wchar_t *s, size_t maxlen)
>>  {
>> +  /* Assert that the entire input is readable.  */
>> +  for (size_t i = 0; i < maxlen; ++i)
>> +    asm volatile ("" :: "r" (s[i]));
>> +
>>    const wchar_t *ret = __wmemchr (s, L'\0', maxlen);
>>    if (ret)
>>      maxlen = ret - s;
>>
>> The resulting crashes demonstrate that the test suite verifies that we
>> do not treat the input as an array (to some degree; there might be
>> scopes in coverage).
>>
>> I think we should document this as a GNU extension.  Thoughts?
> 
> We should absolutely document this. It's an implementation-dependent detail
> that we choose to interpret the standard in this way.
> 

I also think we should get changes into the linux man page project to call
this out so that nobody thinks about changing this again and so the
implementation is clear.

Have we asked Rich what musl does and what he thinks on the topic?

-- 
Cheers,
Carlos.

  reply	other threads:[~2019-11-28 15:58 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-30 10:25 Florian Weimer (Code Review)
2019-10-30 10:44 ` Andreas Schwab
2019-10-30 10:55   ` Florian Weimer
2019-10-30 11:00     ` Andreas Schwab
2019-10-30 11:03       ` Florian Weimer
2019-10-30 11:10         ` Andreas Schwab
2019-10-30 12:01           ` Zack Weinberg
2019-10-30 16:20             ` Andreas Schwab
2019-10-30 16:31               ` Zack Weinberg
2019-10-30 16:47                 ` Andreas Schwab
2019-10-30 16:58                   ` Zack Weinberg
2019-10-30 17:26                     ` Andreas Schwab
2019-10-30 18:12                       ` Zack Weinberg
2019-10-30 18:36                         ` Florian Weimer
2019-10-30 17:24             ` Joseph Myers
2019-11-28  9:43         ` Florian Weimer
2019-11-28 15:56           ` Carlos O'Donell
2019-11-28 15:58             ` Carlos O'Donell [this message]
2019-11-28 18:23               ` Rich Felker
2019-11-28 18:38                 ` Szabolcs Nagy
2019-11-29 18:20                   ` Martin Sebor
2019-11-27 19:08 ` Carlos O'Donell (Code Review)
2019-11-27 19:14 ` Florian Weimer (Code Review)
2019-11-27 22:11 ` Carlos O'Donell (Code Review)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4312fc25-9b4c-9fab-25b1-495a618ab132@redhat.com \
    --to=codonell@redhat.com \
    --cc=dalias@libc.org \
    --cc=fw@deneb.enyo.de \
    --cc=libc-alpha@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).