public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Carlos O'Donell <carlos@redhat.com>
To: Florian Weimer <fweimer@redhat.com>, libc-alpha@sourceware.org
Subject: Re: [PATCH] string: strtok, strtok_r should accept initial NULL subject (bug 16640)
Date: Tue, 8 Nov 2022 08:48:09 -0500	[thread overview]
Message-ID: <83dbe5a2-26f8-b783-edbd-1b85f92b7a80@redhat.com> (raw)
In-Reply-To: <87bkpjm9d4.fsf@oldenburg.str.redhat.com>

On 11/7/22 03:55, Florian Weimer via Libc-alpha wrote:
> The BSD and musl implementations accept an initial NULL subject
> argument.  This also used to be support in glibc on some architectures
> with custom assembler code.

(1) Standards

The ISO C standard pretty clearly says that the first call in the sequence has a
non-null first argument.

(2) Security

From a security stand point I do not want to see a change in behaviour where we
go from failing quickly and immediately at the point we invoke undefined behaviour
to potentially failing far from the point.

We continue to move in this direction across all glibc APIs and we have previously
discussed that we don't want to run with a potentially invalid state for any longer
than required to shut the process down, and the shutdown itself should be as quick
as possible.

(3) Performance/Stability

Terminating the process quickly reduces restart latency, which means the service
can be restarted quickly when an error is encountered.

(4) Compatibility

I don't see any strong justification for why we would change the behaviour, other
than compatibility with musl or certain BSDs. The compatibility of a undefined
behaviour, poorly defined behaviour or implementation defined behaviour needs to
be evaluated on the merits of the individual change. In this case I don't see a
strong justification for exact compatibility in the "first call first argument NULL"
case. I see a strong argument for harmonizing the behaviour and we did that in
the glibc 2.26 release.

Note that this behaviour is not implementation defined, but such behaviour would
need to be documented. This is undefined behaviour.

(5) Existing behaviour.

The glibc implementation behaviour was not uniform across all architectures, and
we fixed that in glibc 2.26.  We have had this behaviour in place for 4 years
across all the architectures, and even longer in the generic code.

In summary:

- I don't see a strong justification to change glibc. This gets a NACK from me.
 
> Tested on i686-linux-gnu and x86_64-linux-gnu.
> 
> ---
>  string/Makefile          |  1 +
>  string/strtok_r.c        |  6 +++++-
>  string/tst-strtok-null.c | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/string/Makefile b/string/Makefile
> index 938f528b8d..a1c6587376 100644
> --- a/string/Makefile
> +++ b/string/Makefile
> @@ -177,6 +177,7 @@ tests := \
>    tst-strfry \
>    tst-strlen \
>    tst-strtok \
> +  tst-strtok-null \
>    tst-strtok_r \
>    tst-strxfrm \
>    tst-strxfrm2 \
> diff --git a/string/strtok_r.c b/string/strtok_r.c
> index fd3a842c99..8342d2ac74 100644
> --- a/string/strtok_r.c
> +++ b/string/strtok_r.c
> @@ -44,7 +44,11 @@ __strtok_r (char *s, const char *delim, char **save_ptr)
>    char *end;
>  
>    if (s == NULL)
> -    s = *save_ptr;
> +    {
> +      if (*save_ptr == NULL)
> +	return NULL;
> +      s = *save_ptr;
> +    }
>  
>    if (*s == '\0')
>      {
> diff --git a/string/tst-strtok-null.c b/string/tst-strtok-null.c
> new file mode 100644
> index 0000000000..2cbc4e5fc4
> --- /dev/null
> +++ b/string/tst-strtok-null.c
> @@ -0,0 +1,32 @@
> +/* Check that strtok and strtok_r accept NULL for the initial subject string.
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stddef.h>
> +#include <string.h>
> +#include <support/check.h>
> +
> +static int
> +do_test (void)
> +{
> +  TEST_COMPARE_STRING (strtok (NULL, ","), NULL);
> +  char *save = NULL;
> +  TEST_COMPARE_STRING (strtok_r (NULL, ",", &save), NULL);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 
> base-commit: 9cc9d61ee12f2f8620d8e0ea3c42af02bf07fe1e
> 

-- 
Cheers,
Carlos.


      reply	other threads:[~2022-11-08 13:48 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-07  8:55 Florian Weimer
2022-11-08 13:48 ` Carlos O'Donell [this message]

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=83dbe5a2-26f8-b783-edbd-1b85f92b7a80@redhat.com \
    --to=carlos@redhat.com \
    --cc=fweimer@redhat.com \
    --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).